[PATCH] ALS: TSL2550 driver move from i2c/chips

[PATCH] ALS: TSL2550 driver move from i2c/chips

[Jonathan Cameron]

Signed-off-by: Jonathan Cameron <jic23-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>

---
Minimal changes made. Untested due to lack of hardware.
All comments welcome.

illuminance is already documented as part of the class.
operating mode and power state are both as per the original
driver. I can't find any documentation for them, but if people
want it I can probably figure out what they are from
the data sheet.  

Might be worth dropping the power state control and moving
over to runtime pm but that is definitely the topic for another
patch.

Does anyone want a patch without using Git's move functionality?

Have copied all the users Jean knows about, but please cc any other
users as this does involve a change to the userspace interface.

Applies on 2.6.31-rc3 with the ALS patches
http://patchwork.kernel.org/patch/49153/

 drivers/als/Kconfig                  |   14 ++++++
 drivers/als/Makefile                 |    2 +
 drivers/{i2c/chips => als}/tsl2550.c |   73 ++++++++++++++++++++++++++++++---
 3 files changed, 82 insertions(+), 7 deletions(-)

diff --git a/drivers/als/Kconfig b/drivers/als/Kconfig
index 200c52b..0c5dbb2 100644
--- a/drivers/als/Kconfig
+++ b/drivers/als/Kconfig
@@ -8,3 +8,17 @@ menuconfig ALS
 	  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.
+
+if ALS
+
+config ALS_TSL2550
+	tristate "Taos TSL2550 ambient light sensor"
+	depends on EXPERIMENTAL
+	help
+	  If you say yes here you get support for the Taos TSL2550
+	  ambient light sensor.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called tsl2550.
+
+endif #ALS
diff --git a/drivers/als/Makefile b/drivers/als/Makefile
index a527197..7be5631 100644
--- a/drivers/als/Makefile
+++ b/drivers/als/Makefile
@@ -3,3 +3,5 @@
 #
 
 obj-$(CONFIG_ALS)		+= als_sys.o
+
+obj-$(CONFIG_ALS_TSL2550)	+= tsl2550.o
\ No newline at end of file
diff --git a/drivers/i2c/chips/tsl2550.c b/drivers/als/tsl2550.c
similarity index 87%
rename from drivers/i2c/chips/tsl2550.c
rename to drivers/als/tsl2550.c
index aa96bd2..6c11695 100644
--- a/drivers/i2c/chips/tsl2550.c
+++ b/drivers/als/tsl2550.c
@@ -24,9 +24,12 @@
 #include <linux/slab.h>
 #include <linux/i2c.h>
 #include <linux/mutex.h>
+#include <linux/err.h>
+#include <linux/idr.h>
+#include <linux/als_sys.h>
 
 #define TSL2550_DRV_NAME	"tsl2550"
-#define DRIVER_VERSION		"1.2"
+#define DRIVER_VERSION		"2"
 
 /*
  * Defines
@@ -44,8 +47,10 @@
  */
 
 struct tsl2550_data {
+	struct device *classdev;
 	struct i2c_client *client;
 	struct mutex update_lock;
+	unsigned int id;
 
 	unsigned int power_state : 1;
 	unsigned int operating_mode : 1;
@@ -60,9 +65,41 @@ static const u8 TSL2550_MODE_RANGE[2] = {
 };
 
 /*
+ * IDR to assign each registered device a unique id
+ */
+static DEFINE_IDR(tsl2550_idr);
+static DEFINE_SPINLOCK(tsl2550_idr_lock);
+#define TSL2550_DEV_MAX 9
+
+/*
  * Management functions
  */
 
+static int tsl2550_get_id(void)
+{
+	int ret, val;
+
+idr_again:
+	if (unlikely(idr_pre_get(&tsl2550_idr, GFP_KERNEL) == 0))
+		return -ENOMEM;
+	spin_lock(&tsl2550_idr_lock);
+	ret = idr_get_new(&tsl2550_idr, NULL, &val);
+	if (unlikely(ret == -EAGAIN))
+		goto idr_again;
+	else if (unlikely(ret))
+		return ret;
+	if (val > TSL2550_DEV_MAX)
+		return -ENOMEM;
+	return val;
+}
+
+static void tsl2550_free_id(int val)
+{
+	spin_lock(&tsl2550_idr_lock);
+	idr_remove(&tsl2550_idr, val);
+	spin_unlock(&tsl2550_idr_lock);
+}
+
 static int tsl2550_set_operating_mode(struct i2c_client *client, int mode)
 {
 	struct tsl2550_data *data = i2c_get_clientdata(client);
@@ -296,13 +333,13 @@ static ssize_t tsl2550_show_lux1_input(struct device *dev,
 	return ret;
 }
 
-static DEVICE_ATTR(lux1_input, S_IRUGO,
+static DEVICE_ATTR(illuminance, S_IRUGO,
 		   tsl2550_show_lux1_input, NULL);
 
 static struct attribute *tsl2550_attributes[] = {
 	&dev_attr_power_state.attr,
 	&dev_attr_operating_mode.attr,
-	&dev_attr_lux1_input.attr,
+	&dev_attr_illuminance.attr,
 	NULL
 };
 
@@ -350,6 +387,7 @@ static int __devinit tsl2550_probe(struct i2c_client *client,
 	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
 	struct tsl2550_data *data;
 	int *opmode, err = 0;
+	char name[9];
 
 	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WRITE_BYTE
 					    | I2C_FUNC_SMBUS_READ_BYTE_DATA)) {
@@ -387,15 +425,31 @@ static int __devinit tsl2550_probe(struct i2c_client *client,
 	if (err)
 		goto exit_kfree;
 
+	data->id = tsl2550_get_id();
+	if (data->id < 0) {
+		err = data->id;
+		goto exit_kfree;
+	}
+	sprintf(name, "tsl2550%d", data->id);
 	/* Register sysfs hooks */
-	err = sysfs_create_group(&client->dev.kobj, &tsl2550_attr_group);
+	data->classdev = als_device_register(&client->dev, name);
+	if (IS_ERR(data->classdev)) {
+		err = PTR_ERR(data->classdev);
+		goto exit_free_id;
+	}
+
+	err = sysfs_create_group(&data->classdev->kobj, &tsl2550_attr_group);
 	if (err)
-		goto exit_kfree;
+		goto exit_unreg;
 
 	dev_info(&client->dev, "support ver. %s enabled\n", DRIVER_VERSION);
 
 	return 0;
 
+exit_unreg:
+	als_device_unregister(data->classdev);
+exit_free_id:
+	tsl2550_free_id(data->id);
 exit_kfree:
 	kfree(data);
 exit:
@@ -404,12 +458,17 @@ exit:
 
 static int __devexit tsl2550_remove(struct i2c_client *client)
 {
-	sysfs_remove_group(&client->dev.kobj, &tsl2550_attr_group);
+	struct tsl2550_data *data = i2c_get_clientdata(client);
+
+	sysfs_remove_group(&data->classdev->kobj, &tsl2550_attr_group);
+	als_device_unregister(data->classdev);
 
 	/* Power down the device */
 	tsl2550_set_power_state(client, 0);
 
-	kfree(i2c_get_clientdata(client));
+	tsl2550_free_id(data->id);
+
+	kfree(data);
 
 	return 0;
 }
-- 
1.6.3.3

Re: [PATCH] ALS: TSL2550 driver move from i2c/chips

[Jonathan Cameron]

Gah, I forgot the I2C dependency as well.

Sorry for the silly errors.
> Signed-off-by: Jonathan Cameron <jic23@cam.ac.uk>
> 
> ---
> Minimal changes made. Untested due to lack of hardware.
> All comments welcome.
> 
> illuminance is already documented as part of the class.
> operating mode and power state are both as per the original
> driver. I can't find any documentation for them, but if people
> want it I can probably figure out what they are from
> the data sheet.  
> 
> Might be worth dropping the power state control and moving
> over to runtime pm but that is definitely the topic for another
> patch.
> 
> Does anyone want a patch without using Git's move functionality?
> 
> Have copied all the users Jean knows about, but please cc any other
> users as this does involve a change to the userspace interface.
> 
> Applies on 2.6.31-rc3 with the ALS patches
> http://patchwork.kernel.org/patch/49153/
> 
>  drivers/als/Kconfig                  |   14 ++++++
>  drivers/als/Makefile                 |    2 +
>  drivers/{i2c/chips => als}/tsl2550.c |   73 ++++++++++++++++++++++++++++++---
>  3 files changed, 82 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/als/Kconfig b/drivers/als/Kconfig
> index 200c52b..0c5dbb2 100644
> --- a/drivers/als/Kconfig
> +++ b/drivers/als/Kconfig
> @@ -8,3 +8,17 @@ menuconfig ALS
>  	  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.
> +
> +if ALS
> +
> +config ALS_TSL2550
> +	tristate "Taos TSL2550 ambient light sensor"
> +	depends on EXPERIMENTAL
> +	help
> +	  If you say yes here you get support for the Taos TSL2550
> +	  ambient light sensor.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called tsl2550.
> +
> +endif #ALS
> diff --git a/drivers/als/Makefile b/drivers/als/Makefile
> index a527197..7be5631 100644
> --- a/drivers/als/Makefile
> +++ b/drivers/als/Makefile
> @@ -3,3 +3,5 @@
>  #
>  
>  obj-$(CONFIG_ALS)		+= als_sys.o
> +
> +obj-$(CONFIG_ALS_TSL2550)	+= tsl2550.o
> \ No newline at end of file
> diff --git a/drivers/i2c/chips/tsl2550.c b/drivers/als/tsl2550.c
> similarity index 87%
> rename from drivers/i2c/chips/tsl2550.c
> rename to drivers/als/tsl2550.c
> index aa96bd2..6c11695 100644
> --- a/drivers/i2c/chips/tsl2550.c
> +++ b/drivers/als/tsl2550.c
> @@ -24,9 +24,12 @@
>  #include <linux/slab.h>
>  #include <linux/i2c.h>
>  #include <linux/mutex.h>
> +#include <linux/err.h>
> +#include <linux/idr.h>
> +#include <linux/als_sys.h>
>  
>  #define TSL2550_DRV_NAME	"tsl2550"
> -#define DRIVER_VERSION		"1.2"
> +#define DRIVER_VERSION		"2"
>  
>  /*
>   * Defines
> @@ -44,8 +47,10 @@
>   */
>  
>  struct tsl2550_data {
> +	struct device *classdev;
>  	struct i2c_client *client;
>  	struct mutex update_lock;
> +	unsigned int id;
>  
>  	unsigned int power_state : 1;
>  	unsigned int operating_mode : 1;
> @@ -60,9 +65,41 @@ static const u8 TSL2550_MODE_RANGE[2] = {
>  };
>  
>  /*
> + * IDR to assign each registered device a unique id
> + */
> +static DEFINE_IDR(tsl2550_idr);
> +static DEFINE_SPINLOCK(tsl2550_idr_lock);
> +#define TSL2550_DEV_MAX 9
> +
> +/*
>   * Management functions
>   */
>  
> +static int tsl2550_get_id(void)
> +{
> +	int ret, val;
> +
> +idr_again:
> +	if (unlikely(idr_pre_get(&tsl2550_idr, GFP_KERNEL) == 0))
> +		return -ENOMEM;
> +	spin_lock(&tsl2550_idr_lock);
> +	ret = idr_get_new(&tsl2550_idr, NULL, &val);
> +	if (unlikely(ret == -EAGAIN))
> +		goto idr_again;
> +	else if (unlikely(ret))
> +		return ret;
> +	if (val > TSL2550_DEV_MAX)
> +		return -ENOMEM;
> +	return val;
> +}
> +
> +static void tsl2550_free_id(int val)
> +{
> +	spin_lock(&tsl2550_idr_lock);
> +	idr_remove(&tsl2550_idr, val);
> +	spin_unlock(&tsl2550_idr_lock);
> +}
> +
>  static int tsl2550_set_operating_mode(struct i2c_client *client, int mode)
>  {
>  	struct tsl2550_data *data = i2c_get_clientdata(client);
> @@ -296,13 +333,13 @@ static ssize_t tsl2550_show_lux1_input(struct device *dev,
>  	return ret;
>  }
>  
> -static DEVICE_ATTR(lux1_input, S_IRUGO,
> +static DEVICE_ATTR(illuminance, S_IRUGO,
>  		   tsl2550_show_lux1_input, NULL);
>  
>  static struct attribute *tsl2550_attributes[] = {
>  	&dev_attr_power_state.attr,
>  	&dev_attr_operating_mode.attr,
> -	&dev_attr_lux1_input.attr,
> +	&dev_attr_illuminance.attr,
>  	NULL
>  };
>  
> @@ -350,6 +387,7 @@ static int __devinit tsl2550_probe(struct i2c_client *client,
>  	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
>  	struct tsl2550_data *data;
>  	int *opmode, err = 0;
> +	char name[9];
>  
>  	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WRITE_BYTE
>  					    | I2C_FUNC_SMBUS_READ_BYTE_DATA)) {
> @@ -387,15 +425,31 @@ static int __devinit tsl2550_probe(struct i2c_client *client,
>  	if (err)
>  		goto exit_kfree;
>  
> +	data->id = tsl2550_get_id();
> +	if (data->id < 0) {
> +		err = data->id;
> +		goto exit_kfree;
> +	}
> +	sprintf(name, "tsl2550%d", data->id);
>  	/* Register sysfs hooks */
> -	err = sysfs_create_group(&client->dev.kobj, &tsl2550_attr_group);
> +	data->classdev = als_device_register(&client->dev, name);
> +	if (IS_ERR(data->classdev)) {
> +		err = PTR_ERR(data->classdev);
> +		goto exit_free_id;
> +	}
> +
> +	err = sysfs_create_group(&data->classdev->kobj, &tsl2550_attr_group);
>  	if (err)
> -		goto exit_kfree;
> +		goto exit_unreg;
>  
>  	dev_info(&client->dev, "support ver. %s enabled\n", DRIVER_VERSION);
>  
>  	return 0;
>  
> +exit_unreg:
> +	als_device_unregister(data->classdev);
> +exit_free_id:
> +	tsl2550_free_id(data->id);
>  exit_kfree:
>  	kfree(data);
>  exit:
> @@ -404,12 +458,17 @@ exit:
>  
>  static int __devexit tsl2550_remove(struct i2c_client *client)
>  {
> -	sysfs_remove_group(&client->dev.kobj, &tsl2550_attr_group);
> +	struct tsl2550_data *data = i2c_get_clientdata(client);
> +
> +	sysfs_remove_group(&data->classdev->kobj, &tsl2550_attr_group);
> +	als_device_unregister(data->classdev);
>  
>  	/* Power down the device */
>  	tsl2550_set_power_state(client, 0);
>  
> -	kfree(i2c_get_clientdata(client));
> +	tsl2550_free_id(data->id);
> +
> +	kfree(data);
>  
>  	return 0;
>  }

Re: [PATCH] ALS: TSL2550 driver move from i2c/chips

[Jonathan Cameron]

Friday afternoon moment....

That id element will need to be signed for any errors on idr allocation
to be detected.  Will fix for next version.
>  struct tsl2550_data {
> +	struct device *classdev;
>  	struct i2c_client *client;
>  	struct mutex update_lock;
> +	unsigned int id;
>  
>  	unsigned int power_state : 1;
>  	unsigned int operating_mode : 1;
>   
....
>  
> +	data->id = tsl2550_get_id();
> +	if (data->id < 0) {
> +		err = data->id;
> +		goto exit_kfree;
> +	}
>   

Jonathan

Re: [PATCH] ALS: TSL2550 driver move from i2c/chips

[Jean Delvare]

Hi Jonathan,

On Fri, 09 Oct 2009 15:37:58 +0100, Jonathan Cameron wrote:
> 
> Signed-off-by: Jonathan Cameron <jic23-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>
> 
> ---
> Minimal changes made. Untested due to lack of hardware.
> All comments welcome.

Thanks for working on this. I can do any amount of testing you want, as
I have received a TSL2550 evaluation module from TAOS.

> illuminance is already documented as part of the class.
> operating mode and power state are both as per the original
> driver. I can't find any documentation for them, but if people
> want it I can probably figure out what they are from
> the data sheet.

The operating mode selects the measurable range. Standard range is from
0 to 1846 lux, extended range is from 0 to 9230 lux, with a resolution
divided by 5. Extended mode is also 5 times faster.

What do we want to do with this? I am open to suggestions. There are
several possibilities. The operating mode could be provided as platform
data and stay internal to the driver. Or we can leave is visible to
user-space, in which case I'd recommend that we do so in terms of
"range" rather than "mode", so that other drivers can use the same
convention, whatever it becomes. For example, one would write the range
of values he/she wants to be able to measure and the driver would put
the device in the most appropriate mode.

Alternatively (or additionally), we could implement an automatic mode
which would change the mode dynamically based on the previous
measurement. I've done that for one hwmon driver (for fan speed
measurement) and it works very well, if implemented properly.

> Might be worth dropping the power state control and moving
> over to runtime pm but that is definitely the topic for another
> patch.

Power state control is already integrated into the PM framework
(suspend and resume, is there more?) The sysfs entry is to allow a
manual control on top of it. I don't much like having a custom sysfs
entry for this, but I don't know if there is a standard way to achieve
the same?

> Does anyone want a patch without using Git's move functionality?

Yes please!

> Have copied all the users Jean knows about, but please cc any other
> users as this does involve a change to the userspace interface.
> 
> Applies on 2.6.31-rc3 with the ALS patches
> http://patchwork.kernel.org/patch/49153/
> 
>  drivers/als/Kconfig                  |   14 ++++++
>  drivers/als/Makefile                 |    2 +
>  drivers/{i2c/chips => als}/tsl2550.c |   73 ++++++++++++++++++++++++++++++---
>  3 files changed, 82 insertions(+), 7 deletions(-)

-- 
Jean Delvare

Re: [PATCH] ALS: TSL2550 driver move from i2c/chips

[Jonathan Cameron]

Hi Jean,
> 
> On Fri, 09 Oct 2009 15:37:58 +0100, Jonathan Cameron wrote:
>> Signed-off-by: Jonathan Cameron <jic23-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>
>>
>> ---
>> Minimal changes made. Untested due to lack of hardware.
>> All comments welcome.
> 
> Thanks for working on this. I can do any amount of testing you want, as
> I have received a TSL2550 evaluation module from TAOS.
> 
>> illuminance is already documented as part of the class.
>> operating mode and power state are both as per the original
>> driver. I can't find any documentation for them, but if people
>> want it I can probably figure out what they are from
>> the data sheet.
> 
> The operating mode selects the measurable range. Standard range is from
> 0 to 1846 lux, extended range is from 0 to 9230 lux, with a resolution
> divided by 5. Extended mode is also 5 times faster.
> 
> What do we want to do with this? I am open to suggestions. There are
> several possibilities. The operating mode could be provided as platform
> data and stay internal to the driver. Or we can leave is visible to
> user-space, in which case I'd recommend that we do so in terms of
> "range" rather than "mode", so that other drivers can use the same
> convention, whatever it becomes. For example, one would write the range
> of values he/she wants to be able to measure and the driver would put
> the device in the most appropriate mode.
That sounds like a sensible approach.  For now I guess a sysfs parameter
like illuminance_range_max would do the job and we can add a min for any
device that features an offset?
> 
> Alternatively (or additionally), we could implement an automatic mode
> which would change the mode dynamically based on the previous
> measurement. I've done that for one hwmon driver (for fan speed
> measurement) and it works very well, if implemented properly.
Could do.  Maybe put it in as manual for now and leave the automatic
mode for a later date?
> 
>> Might be worth dropping the power state control and moving
>> over to runtime pm but that is definitely the topic for another
>> patch.
> 
> Power state control is already integrated into the PM framework
> (suspend and resume, is there more?) The sysfs entry is to allow a
> manual control on top of it. I don't much like having a custom sysfs
> entry for this, but I don't know if there is a standard way to achieve
> the same?
Not directly as far as I know. The runtime pm is more about buses figuring
out if they can suspend (and take their children into suspend as well).
Personally I haven't looked at the code for those buses that implement it
yet to see exactly how it relates to fine grained control as here.
For now lets leave this as it is with a plan to fix it later.

I do notice that usb supports this sort of control by adding sysfs entries to
the existing power sysfs attribute group.
(curiously I think the relevant one is called level, so we would have
/sys/bus/i2c/devices/*/power/level

We could do the same... 
> 
>> Does anyone want a patch without using Git's move functionality?
> 
> Yes please!
Will post next version. (thanks for the review based on the move version!)
> 
>> Have copied all the users Jean knows about, but please cc any other
>> users as this does involve a change to the userspace interface.
>>
>> Applies on 2.6.31-rc3 with the ALS patches
>> http://patchwork.kernel.org/patch/49153/
>>
>>  drivers/als/Kconfig                  |   14 ++++++
>>  drivers/als/Makefile                 |    2 +
>>  drivers/{i2c/chips => als}/tsl2550.c |   73 ++++++++++++++++++++++++++++++---
>>  3 files changed, 82 insertions(+), 7 deletions(-)
> 

Re: [PATCH] ALS: TSL2550 driver move from i2c/chips

[Jean Delvare]

On Mon, 12 Oct 2009 16:13:01 +0100, Jonathan Cameron wrote:
> Hi Jean,
> > 
> > On Fri, 09 Oct 2009 15:37:58 +0100, Jonathan Cameron wrote:
> >> Signed-off-by: Jonathan Cameron <jic23-KWPb1pKIrIJaa/9Udqfwiw@public.gmane.org>
> >>
> >> ---
> >> Minimal changes made. Untested due to lack of hardware.
> >> All comments welcome.
> > 
> > Thanks for working on this. I can do any amount of testing you want, as
> > I have received a TSL2550 evaluation module from TAOS.
> > 
> >> illuminance is already documented as part of the class.
> >> operating mode and power state are both as per the original
> >> driver. I can't find any documentation for them, but if people
> >> want it I can probably figure out what they are from
> >> the data sheet.
> > 
> > The operating mode selects the measurable range. Standard range is from
> > 0 to 1846 lux, extended range is from 0 to 9230 lux, with a resolution
> > divided by 5. Extended mode is also 5 times faster.
> > 
> > What do we want to do with this? I am open to suggestions. There are
> > several possibilities. The operating mode could be provided as platform
> > data and stay internal to the driver. Or we can leave is visible to
> > user-space, in which case I'd recommend that we do so in terms of
> > "range" rather than "mode", so that other drivers can use the same
> > convention, whatever it becomes. For example, one would write the range
> > of values he/she wants to be able to measure and the driver would put
> > the device in the most appropriate mode.
>
> That sounds like a sensible approach.  For now I guess a sysfs parameter
> like illuminance_range_max would do the job and we can add a min for any
> device that features an offset?

Yes, this sounds sensible.

> > Alternatively (or additionally), we could implement an automatic mode
> > which would change the mode dynamically based on the previous
> > measurement. I've done that for one hwmon driver (for fan speed
> > measurement) and it works very well, if implemented properly.
>
> Could do.  Maybe put it in as manual for now and leave the automatic
> mode for a later date?

Yes. Note that the automatic mode may have to be optional, too. I can
imagine use cases where one prefers to stick to a given range and spare
I2C bus cycles.

> >> Might be worth dropping the power state control and moving
> >> over to runtime pm but that is definitely the topic for another
> >> patch.
> > 
> > Power state control is already integrated into the PM framework
> > (suspend and resume, is there more?) The sysfs entry is to allow a
> > manual control on top of it. I don't much like having a custom sysfs
> > entry for this, but I don't know if there is a standard way to achieve
> > the same?
>
> Not directly as far as I know. The runtime pm is more about buses figuring
> out if they can suspend (and take their children into suspend as well).
> Personally I haven't looked at the code for those buses that implement it
> yet to see exactly how it relates to fine grained control as here.
> For now lets leave this as it is with a plan to fix it later.
> 
> I do notice that usb supports this sort of control by adding sysfs entries to
> the existing power sysfs attribute group.
> (curiously I think the relevant one is called level, so we would have
> /sys/bus/i2c/devices/*/power/level
> 
> We could do the same...

Are these levels part of the USB specification? In the case of the
TSL2550, it has very little to do with the I2C specification. It's just
about enabling/disabling light sensing, much like you'd disable a
temperature sensor when you don't need it, to save power. That kind of
power level switching is not bus-specific. You can see it as either
device-specific, if you go into the details, or general, if you don't.

-- 
Jean Delvare

Re: [PATCH] ALS: TSL2550 driver move from i2c/chips

[Jean Delvare]

On Mon, 12 Oct 2009 17:38:24 +0200, Jean Delvare wrote:
> On Mon, 12 Oct 2009 16:13:01 +0100, Jonathan Cameron wrote:
> > > The operating mode selects the measurable range. Standard range is from
> > > 0 to 1846 lux, extended range is from 0 to 9230 lux, with a resolution
> > > divided by 5. Extended mode is also 5 times faster.
> > > 
> > > What do we want to do with this? I am open to suggestions. There are
> > > several possibilities. The operating mode could be provided as platform
> > > data and stay internal to the driver. Or we can leave is visible to
> > > user-space, in which case I'd recommend that we do so in terms of
> > > "range" rather than "mode", so that other drivers can use the same
> > > convention, whatever it becomes. For example, one would write the range
> > > of values he/she wants to be able to measure and the driver would put
> > > the device in the most appropriate mode.
> >
> > That sounds like a sensible approach.  For now I guess a sysfs parameter
> > like illuminance_range_max would do the job and we can add a min for any
> > device that features an offset?
> 
> Yes, this sounds sensible.

Following up publicly so that everyone interested knows what's going on.

Jonathan and me tested the above idea on the tsl2550 driver, and it
turns out that it doesn't work well. The TSL2550 computes the lux of
visible light by computing a difference between total light (infra-red +
visible) and infra-red light. When there's a lot of infra-red light, it
is very easy to saturate the light sensors in standard mode, even
though the amount of visible light is low.

The bottom line is that one may need to reduce the integration time
(which is what the "extended mode" is really about) even if the result
fits in the standard mode's range. In the light of this, it seems
wrong to hide the mode behind the name "illuminance_range_max". It
seems better to export the exposure time as is, readable and writable.
Remains to agree on a file name and unit.

-- 
Jean Delvare

Re: [PATCH] ALS: TSL2550 driver move from i2c/chips

[Jean Delvare]

On Fri, 09 Oct 2009 15:37:58 +0100, Jonathan Cameron wrote:
>  drivers/als/Kconfig                  |   14 ++++++
>  drivers/als/Makefile                 |    2 +
>  drivers/{i2c/chips => als}/tsl2550.c |   73 ++++++++++++++++++++++++++++++---
>  3 files changed, 82 insertions(+), 7 deletions(-)

Review:

> diff --git a/drivers/als/Kconfig b/drivers/als/Kconfig
> index 200c52b..0c5dbb2 100644
> --- a/drivers/als/Kconfig
> +++ b/drivers/als/Kconfig
> @@ -8,3 +8,17 @@ menuconfig ALS
>  	  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.
> +
> +if ALS
> +
> +config ALS_TSL2550
> +	tristate "Taos TSL2550 ambient light sensor"
> +	depends on EXPERIMENTAL

As you found out already, you need to depend on I2C as well.

> +	help
> +	  If you say yes here you get support for the Taos TSL2550
> +	  ambient light sensor.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called tsl2550.
> +
> +endif #ALS
> diff --git a/drivers/als/Makefile b/drivers/als/Makefile
> index a527197..7be5631 100644
> --- a/drivers/als/Makefile
> +++ b/drivers/als/Makefile
> @@ -3,3 +3,5 @@
>  #
>  
>  obj-$(CONFIG_ALS)		+= als_sys.o
> +
> +obj-$(CONFIG_ALS_TSL2550)	+= tsl2550.o
> \ No newline at end of file
> diff --git a/drivers/i2c/chips/tsl2550.c b/drivers/als/tsl2550.c
> similarity index 87%
> rename from drivers/i2c/chips/tsl2550.c
> rename to drivers/als/tsl2550.c
> index aa96bd2..6c11695 100644
> --- a/drivers/i2c/chips/tsl2550.c
> +++ b/drivers/als/tsl2550.c
> @@ -24,9 +24,12 @@
>  #include <linux/slab.h>
>  #include <linux/i2c.h>
>  #include <linux/mutex.h>
> +#include <linux/err.h>
> +#include <linux/idr.h>
> +#include <linux/als_sys.h>
>  
>  #define TSL2550_DRV_NAME	"tsl2550"
> -#define DRIVER_VERSION		"1.2"
> +#define DRIVER_VERSION		"2"

"2.0"?

>  
>  /*
>   * Defines
> @@ -44,8 +47,10 @@
>   */
>  
>  struct tsl2550_data {
> +	struct device *classdev;
>  	struct i2c_client *client;
>  	struct mutex update_lock;
> +	unsigned int id;
>  
>  	unsigned int power_state : 1;
>  	unsigned int operating_mode : 1;
> @@ -60,9 +65,41 @@ static const u8 TSL2550_MODE_RANGE[2] = {
>  };
>  
>  /*
> + * IDR to assign each registered device a unique id
> + */
> +static DEFINE_IDR(tsl2550_idr);
> +static DEFINE_SPINLOCK(tsl2550_idr_lock);
> +#define TSL2550_DEV_MAX 9

Such an arbitrary limit is simply not acceptable.

> +
> +/*
>   * Management functions
>   */
>  
> +static int tsl2550_get_id(void)
> +{
> +	int ret, val;
> +
> +idr_again:
> +	if (unlikely(idr_pre_get(&tsl2550_idr, GFP_KERNEL) == 0))
> +		return -ENOMEM;
> +	spin_lock(&tsl2550_idr_lock);
> +	ret = idr_get_new(&tsl2550_idr, NULL, &val);
> +	if (unlikely(ret == -EAGAIN))
> +		goto idr_again;
> +	else if (unlikely(ret))
> +		return ret;
> +	if (val > TSL2550_DEV_MAX)
> +		return -ENOMEM;
> +	return val;
> +}
> +
> +static void tsl2550_free_id(int val)
> +{
> +	spin_lock(&tsl2550_idr_lock);
> +	idr_remove(&tsl2550_idr, val);
> +	spin_unlock(&tsl2550_idr_lock);
> +}

Having to implement this in _every_ ALS driver sounds wrong and
painful. If uniqueness of any kind must be provided, it should be
handled by the ALS core and not by individual drivers, otherwise you
can be certain that at least one driver will get it wrong someday.

I'd imaging that als-class devices are simply named als%u. Just like
hwmon devices are named hwmon%u, input devices are names input%u and
event%u, etc. I don't know of any class pushing the naming down to its
drivers, do you? The only example I can remember are i2c drivers back
in year 1999, when they were part of lm-sensors.I have personally put
an end to this years ago.

> +
>  static int tsl2550_set_operating_mode(struct i2c_client *client, int mode)
>  {
>  	struct tsl2550_data *data = i2c_get_clientdata(client);
> @@ -296,13 +333,13 @@ static ssize_t tsl2550_show_lux1_input(struct device *dev,
>  	return ret;
>  }
>  
> -static DEVICE_ATTR(lux1_input, S_IRUGO,
> +static DEVICE_ATTR(illuminance, S_IRUGO,
>  		   tsl2550_show_lux1_input, NULL);

Question: if I have a light sensing chip with two sensors, how are we
going to handle it? Will we register 2 als class devices? The initial
naming convention had the advantage that you could have more than one
sensor per device, but I don't know if it is desirable in practice.

>  
>  static struct attribute *tsl2550_attributes[] = {
>  	&dev_attr_power_state.attr,
>  	&dev_attr_operating_mode.attr,
> -	&dev_attr_lux1_input.attr,
> +	&dev_attr_illuminance.attr,
>  	NULL
>  };
>  
> @@ -350,6 +387,7 @@ static int __devinit tsl2550_probe(struct i2c_client *client,
>  	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
>  	struct tsl2550_data *data;
>  	int *opmode, err = 0;
> +	char name[9];
>  
>  	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WRITE_BYTE
>  					    | I2C_FUNC_SMBUS_READ_BYTE_DATA)) {
> @@ -387,15 +425,31 @@ static int __devinit tsl2550_probe(struct i2c_client *client,
>  	if (err)
>  		goto exit_kfree;
>  
> +	data->id = tsl2550_get_id();
> +	if (data->id < 0) {
> +		err = data->id;
> +		goto exit_kfree;
> +	}
> +	sprintf(name, "tsl2550%d", data->id);

Please no. For one thing you should always use snprintf and not sprintf
when writing to such small buffers. It's way too easy to get things
wrong otherwise. And you really want a separator between the chip name
and the id, because "tsl25500" will be confusing as hell.

Not that these comments of mine really matter, as I don't think the
above code should stay at all.

>  	/* Register sysfs hooks */
> -	err = sysfs_create_group(&client->dev.kobj, &tsl2550_attr_group);
> +	data->classdev = als_device_register(&client->dev, name);
> +	if (IS_ERR(data->classdev)) {
> +		err = PTR_ERR(data->classdev);
> +		goto exit_free_id;
> +	}
> +
> +	err = sysfs_create_group(&data->classdev->kobj, &tsl2550_attr_group);
>  	if (err)
> -		goto exit_kfree;
> +		goto exit_unreg;
>  
>  	dev_info(&client->dev, "support ver. %s enabled\n", DRIVER_VERSION);
>  
>  	return 0;
>  
> +exit_unreg:
> +	als_device_unregister(data->classdev);
> +exit_free_id:
> +	tsl2550_free_id(data->id);
>  exit_kfree:
>  	kfree(data);
>  exit:
> @@ -404,12 +458,17 @@ exit:
>  
>  static int __devexit tsl2550_remove(struct i2c_client *client)
>  {
> -	sysfs_remove_group(&client->dev.kobj, &tsl2550_attr_group);
> +	struct tsl2550_data *data = i2c_get_clientdata(client);
> +
> +	sysfs_remove_group(&data->classdev->kobj, &tsl2550_attr_group);
> +	als_device_unregister(data->classdev);
>  
>  	/* Power down the device */
>  	tsl2550_set_power_state(client, 0);
>  
> -	kfree(i2c_get_clientdata(client));
> +	tsl2550_free_id(data->id);
> +
> +	kfree(data);
>  
>  	return 0;
>  }


-- 
Jean Delvare

Re: [PATCH] ALS: TSL2550 driver move from i2c/chips

[Jonathan Cameron]

For those not following this driver, Jean does raise the issue
of device naming again and I think we really need to settle
this one before progressing with ALS in general.

> 
> "2.0"?
True, that would be consistent.
> 
>>  
>>  /*
>>   * Defines
>> @@ -44,8 +47,10 @@
>>   */
>>  
>>  struct tsl2550_data {
>> +	struct device *classdev;
>>  	struct i2c_client *client;
>>  	struct mutex update_lock;
>> +	unsigned int id;
>>  
>>  	unsigned int power_state : 1;
>>  	unsigned int operating_mode : 1;
>> @@ -60,9 +65,41 @@ static const u8 TSL2550_MODE_RANGE[2] = {
>>  };
>>  
>>  /*
>> + * IDR to assign each registered device a unique id
>> + */
>> +static DEFINE_IDR(tsl2550_idr);
>> +static DEFINE_SPINLOCK(tsl2550_idr_lock);
>> +#define TSL2550_DEV_MAX 9
> 
> Such an arbitrary limit is simply not acceptable.
Fair enough, but it is based on restricting the size
of the char array needed for the name when registering
with als.  Hence single digit decimal maximum.
Do you suggest leaving it unrestricted (which makes code
a little fiddly given variable size of max idr) or some other
limit?
 
>> +
>> +/*
>>   * Management functions
>>   */
>>  
>> +static int tsl2550_get_id(void)
>> +{
>> +	int ret, val;
>> +
>> +idr_again:
>> +	if (unlikely(idr_pre_get(&tsl2550_idr, GFP_KERNEL) == 0))
>> +		return -ENOMEM;
>> +	spin_lock(&tsl2550_idr_lock);
>> +	ret = idr_get_new(&tsl2550_idr, NULL, &val);
>> +	if (unlikely(ret == -EAGAIN))
>> +		goto idr_again;
>> +	else if (unlikely(ret))
>> +		return ret;
>> +	if (val > TSL2550_DEV_MAX)
>> +		return -ENOMEM;
>> +	return val;
>> +}
>> +
>> +static void tsl2550_free_id(int val)
>> +{
>> +	spin_lock(&tsl2550_idr_lock);
>> +	idr_remove(&tsl2550_idr, val);
>> +	spin_unlock(&tsl2550_idr_lock);
>> +}
> 
> Having to implement this in _every_ ALS driver sounds wrong and
> painful. If uniqueness of any kind must be provided, it should be
> handled by the ALS core and not by individual drivers, otherwise you
> can be certain that at least one driver will get it wrong someday.
I agree. The reason this originally occurred is that the acpi layer
apparently doesn't allow for simultaneous probing of multiple drivers
and hence can get away with a simple counter and no locking.
> 
> I'd imaging that als-class devices are simply named als%u. Just like
> hwmon devices are named hwmon%u, input devices are names input%u and
> event%u, etc. I don't know of any class pushing the naming down to its
> drivers, do you? The only example I can remember are i2c drivers back
> in year 1999, when they were part of lm-sensors.I have personally put
> an end to this years ago.
This debate started in the als thread. Personally I couldn't care less
either way but it does need to be put to bed before that subsystem merges.
To my mind either approach is trivially handled in a userspace library
so it doesn't matter.  I don't suppose you can remember what the original
reasons for squashing this naming approach were?
> 
>> +
>>  static int tsl2550_set_operating_mode(struct i2c_client *client, int mode)
>>  {
>>  	struct tsl2550_data *data = i2c_get_clientdata(client);
>> @@ -296,13 +333,13 @@ static ssize_t tsl2550_show_lux1_input(struct device *dev,
>>  	return ret;
>>  }
>>  
>> -static DEVICE_ATTR(lux1_input, S_IRUGO,
>> +static DEVICE_ATTR(illuminance, S_IRUGO,
>>  		   tsl2550_show_lux1_input, NULL);
> 
> Question: if I have a light sensing chip with two sensors, how are we
> going to handle it? Will we register 2 als class devices? The initial
> naming convention had the advantage that you could have more than one
> sensor per device, but I don't know if it is desirable in practice.
This only becomes and issue if we have two sensors measuring illuminance
(or approximation of it).  The only two sensor chips I know of have one
broadband and one in the infrared tsl2561 and I think the isl chip with
a driver currently in misc.  The combination of these two are needed to
calculate illuminance.  Some of the original discussion went into how
to support separate access to the individual sensors.  Decision was to
leave that question until it becomes relevant.  Basically we would put
the current drivers in just supporting illuminance and see if anyone asked
for furthers support.  One tricky aspect is what the units should be for
particular frequency ranges. At least illuminance is cleanly defined
(even if chips are only fairly coarsely approximating it. 
> 
>>  
>>  static struct attribute *tsl2550_attributes[] = {
>>  	&dev_attr_power_state.attr,
>>  	&dev_attr_operating_mode.attr,
>> -	&dev_attr_lux1_input.attr,
>> +	&dev_attr_illuminance.attr,
>>  	NULL
>>  };
>>  
>> @@ -350,6 +387,7 @@ static int __devinit tsl2550_probe(struct i2c_client *client,
>>  	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
>>  	struct tsl2550_data *data;
>>  	int *opmode, err = 0;
>> +	char name[9];
>>  
>>  	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WRITE_BYTE
>>  					    | I2C_FUNC_SMBUS_READ_BYTE_DATA)) {
>> @@ -387,15 +425,31 @@ static int __devinit tsl2550_probe(struct i2c_client *client,
>>  	if (err)
>>  		goto exit_kfree;
>>  
>> +	data->id = tsl2550_get_id();
>> +	if (data->id < 0) {
>> +		err = data->id;
>> +		goto exit_kfree;
>> +	}
>> +	sprintf(name, "tsl2550%d", data->id);
> 
> Please no. For one thing you should always use snprintf and not sprintf
> when writing to such small buffers. It's way too easy to get things
> wrong otherwise. And you really want a separator between the chip name
> and the id, because "tsl25500" will be confusing as hell.
The length is fine with the restriction above, but I agree we need a separation
(hadn't really thought this through!).
> 
> Not that these comments of mine really matter, as I don't think the
> above code should stay at all.
That's fair enough and I'm inclined to agree.
> 
>>  	/* Register sysfs hooks */
>> -	err = sysfs_create_group(&client->dev.kobj, &tsl2550_attr_group);
>> +	data->classdev = als_device_register(&client->dev, name);
>> +	if (IS_ERR(data->classdev)) {
>> +		err = PTR_ERR(data->classdev);
>> +		goto exit_free_id;
>> +	}
>> +
>> +	err = sysfs_create_group(&data->classdev->kobj, &tsl2550_attr_group);
>>  	if (err)
>> -		goto exit_kfree;
>> +		goto exit_unreg;
>>  
>>  	dev_info(&client->dev, "support ver. %s enabled\n", DRIVER_VERSION);
>>  
>>  	return 0;
>>  
>> +exit_unreg:
>> +	als_device_unregister(data->classdev);
>> +exit_free_id:
>> +	tsl2550_free_id(data->id);
>>  exit_kfree:
>>  	kfree(data);
>>  exit:
>> @@ -404,12 +458,17 @@ exit:
>>  
>>  static int __devexit tsl2550_remove(struct i2c_client *client)
>>  {
>> -	sysfs_remove_group(&client->dev.kobj, &tsl2550_attr_group);
>> +	struct tsl2550_data *data = i2c_get_clientdata(client);
>> +
>> +	sysfs_remove_group(&data->classdev->kobj, &tsl2550_attr_group);
>> +	als_device_unregister(data->classdev);
>>  
>>  	/* Power down the device */
>>  	tsl2550_set_power_state(client, 0);
>>  
>> -	kfree(i2c_get_clientdata(client));
>> +	tsl2550_free_id(data->id);
>> +
>> +	kfree(data);
>>  
>>  	return 0;
>>  }
> 
> 

Re: [PATCH] ALS: TSL2550 driver move from i2c/chips

[Jean Delvare]

Hi Jonathan,

On Mon, 12 Oct 2009 15:19:07 +0100, Jonathan Cameron wrote:
> >> @@ -60,9 +65,41 @@ static const u8 TSL2550_MODE_RANGE[2] = {
> >>  };
> >>  
> >>  /*
> >> + * IDR to assign each registered device a unique id
> >> + */
> >> +static DEFINE_IDR(tsl2550_idr);
> >> +static DEFINE_SPINLOCK(tsl2550_idr_lock);
> >> +#define TSL2550_DEV_MAX 9
> > 
> > Such an arbitrary limit is simply not acceptable.
>
> Fair enough, but it is based on restricting the size
> of the char array needed for the name when registering
> with als.  Hence single digit decimal maximum.
> Do you suggest leaving it unrestricted (which makes code
> a little fiddly given variable size of max idr) or some other
> limit?

The name size really isn't an issue. You won't notice 16 bytes instead
of 9. And it's not like we'll have millions of these devices.

> >> +static int tsl2550_get_id(void)
> >> +{
> >> +	int ret, val;
> >> +
> >> +idr_again:
> >> +	if (unlikely(idr_pre_get(&tsl2550_idr, GFP_KERNEL) == 0))
> >> +		return -ENOMEM;
> >> +	spin_lock(&tsl2550_idr_lock);
> >> +	ret = idr_get_new(&tsl2550_idr, NULL, &val);
> >> +	if (unlikely(ret == -EAGAIN))
> >> +		goto idr_again;
> >> +	else if (unlikely(ret))
> >> +		return ret;
> >> +	if (val > TSL2550_DEV_MAX)
> >> +		return -ENOMEM;
> >> +	return val;
> >> +}
> >> +
> >> +static void tsl2550_free_id(int val)
> >> +{
> >> +	spin_lock(&tsl2550_idr_lock);
> >> +	idr_remove(&tsl2550_idr, val);
> >> +	spin_unlock(&tsl2550_idr_lock);
> >> +}
> > 
> > Having to implement this in _every_ ALS driver sounds wrong and
> > painful. If uniqueness of any kind must be provided, it should be
> > handled by the ALS core and not by individual drivers, otherwise you
> > can be certain that at least one driver will get it wrong someday.
>
> I agree. The reason this originally occurred is that the acpi layer
> apparently doesn't allow for simultaneous probing of multiple drivers
> and hence can get away with a simple counter and no locking.
>
> > I'd imaging that als-class devices are simply named als%u. Just like
> > hwmon devices are named hwmon%u, input devices are names input%u and
> > event%u, etc. I don't know of any class pushing the naming down to its
> > drivers, do you? The only example I can remember are i2c drivers back
> > in year 1999, when they were part of lm-sensors.I have personally put
> > an end to this years ago.
>
> This debate started in the als thread. Personally I couldn't care less
> either way but it does need to be put to bed before that subsystem merges.
> To my mind either approach is trivially handled in a userspace library
> so it doesn't matter.  I don't suppose you can remember what the original
> reasons for squashing this naming approach were?

Code duplication. Having the same unique-id handling code repeated in
50 drivers was no fun, as it did not add any value compared to a
central handling.

> >> @@ -296,13 +333,13 @@ static ssize_t tsl2550_show_lux1_input(struct device *dev,
> >>  	return ret;
> >>  }
> >>  
> >> -static DEVICE_ATTR(lux1_input, S_IRUGO,
> >> +static DEVICE_ATTR(illuminance, S_IRUGO,
> >>  		   tsl2550_show_lux1_input, NULL);
> > 
> > Question: if I have a light sensing chip with two sensors, how are we
> > going to handle it? Will we register 2 als class devices? The initial
> > naming convention had the advantage that you could have more than one
> > sensor per device, but I don't know if it is desirable in practice.
>
> This only becomes and issue if we have two sensors measuring illuminance
> (or approximation of it).  The only two sensor chips I know of have one
> broadband and one in the infrared tsl2561 and I think the isl chip with
> a driver currently in misc.  The combination of these two are needed to
> calculate illuminance.  Some of the original discussion went into how
> to support separate access to the individual sensors.  Decision was to
> leave that question until it becomes relevant.  Basically we would put
> the current drivers in just supporting illuminance and see if anyone asked
> for furthers support.  One tricky aspect is what the units should be for
> particular frequency ranges. At least illuminance is cleanly defined
> (even if chips are only fairly coarsely approximating it. 

Hmm, this isn't the point I was raising. I was thinking of a light
sensor chip which would sense light in different locations. Think chip
with remote sensors. This is done frequently for thermal sensors, so I
guess it could be done for light sensors as well. A practical
application could be sensing the ambient light on two sides of an
object, so that you get the correct measurement regardless of the
position.

I'm not saying such chips exist, I really don't know. I am just raising
the question of how we'd handle them if they do. The current naming
scheme implies that we'd need a separate als instance for each sensor,
and I want you to be aware of this.

> >>  static struct attribute *tsl2550_attributes[] = {
> >>  	&dev_attr_power_state.attr,
> >>  	&dev_attr_operating_mode.attr,
> >> -	&dev_attr_lux1_input.attr,
> >> +	&dev_attr_illuminance.attr,
> >>  	NULL
> >>  };
> >>  
> >> @@ -350,6 +387,7 @@ static int __devinit tsl2550_probe(struct i2c_client *client,
> >>  	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
> >>  	struct tsl2550_data *data;
> >>  	int *opmode, err = 0;
> >> +	char name[9];
> >>  
> >>  	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WRITE_BYTE
> >>  					    | I2C_FUNC_SMBUS_READ_BYTE_DATA)) {
> >> @@ -387,15 +425,31 @@ static int __devinit tsl2550_probe(struct i2c_client *client,
> >>  	if (err)
> >>  		goto exit_kfree;
> >>  
> >> +	data->id = tsl2550_get_id();
> >> +	if (data->id < 0) {
> >> +		err = data->id;
> >> +		goto exit_kfree;
> >> +	}
> >> +	sprintf(name, "tsl2550%d", data->id);
> > 
> > Please no. For one thing you should always use snprintf and not sprintf
> > when writing to such small buffers. It's way too easy to get things
> > wrong otherwise. And you really want a separator between the chip name
> > and the id, because "tsl25500" will be confusing as hell.
>
> The length is fine with the restriction above, but I agree we need a separation
> (hadn't really thought this through!).

The length is fine until someone needs more sensors, changes
TSL2550_DEV_MAX to 16, and then the code crashes and nobody knows why.
You can't assume that other developers won't touch your code. Thus it
is much better to handle a given limitation in a single place. Here you
can simply check the value returned by snprintf() and then you know if
your name was correctly set.

Again, not that it matters, because the name buffer should simply be
large enough that it always fits.

-- 
Jean Delvare

Re: [PATCH] ALS: TSL2550 driver move from i2c/chips

[Jonathan Cameron]

Jean Delvare wrote:
> Hi Jonathan,
> 
> On Mon, 12 Oct 2009 15:19:07 +0100, Jonathan Cameron wrote:
>>>> @@ -60,9 +65,41 @@ static const u8 TSL2550_MODE_RANGE[2] = {
>>>>  };
>>>>  
>>>>  /*
>>>> + * IDR to assign each registered device a unique id
>>>> + */
>>>> +static DEFINE_IDR(tsl2550_idr);
>>>> +static DEFINE_SPINLOCK(tsl2550_idr_lock);
>>>> +#define TSL2550_DEV_MAX 9
>>> Such an arbitrary limit is simply not acceptable.
>> Fair enough, but it is based on restricting the size
>> of the char array needed for the name when registering
>> with als.  Hence single digit decimal maximum.
>> Do you suggest leaving it unrestricted (which makes code
>> a little fiddly given variable size of max idr) or some other
>> limit?
> 
> The name size really isn't an issue. You won't notice 16 bytes instead
> of 9. And it's not like we'll have millions of these devices.
I agree, it's merely a question of picking some suitable limit. IDR's
on a 64 bit box will do something in the ball park of 2e18 which might
be an excessive limit ;) I'll leave this be for next version on basis
I'm in favour of moving this into the core and hence as you said this
will be irrelevant here anyway.
>>>> +static int tsl2550_get_id(void)
>>>> +{
>>>> +	int ret, val;
>>>> +
>>>> +idr_again:
>>>> +	if (unlikely(idr_pre_get(&tsl2550_idr, GFP_KERNEL) == 0))
>>>> +		return -ENOMEM;
>>>> +	spin_lock(&tsl2550_idr_lock);
>>>> +	ret = idr_get_new(&tsl2550_idr, NULL, &val);
>>>> +	if (unlikely(ret == -EAGAIN))
>>>> +		goto idr_again;
>>>> +	else if (unlikely(ret))
>>>> +		return ret;
>>>> +	if (val > TSL2550_DEV_MAX)
>>>> +		return -ENOMEM;
>>>> +	return val;
>>>> +}
>>>> +
>>>> +static void tsl2550_free_id(int val)
>>>> +{
>>>> +	spin_lock(&tsl2550_idr_lock);
>>>> +	idr_remove(&tsl2550_idr, val);
>>>> +	spin_unlock(&tsl2550_idr_lock);
>>>> +}
>>> Having to implement this in _every_ ALS driver sounds wrong and
>>> painful. If uniqueness of any kind must be provided, it should be
>>> handled by the ALS core and not by individual drivers, otherwise you
>>> can be certain that at least one driver will get it wrong someday.
>> I agree. The reason this originally occurred is that the acpi layer
>> apparently doesn't allow for simultaneous probing of multiple drivers
>> and hence can get away with a simple counter and no locking.
>>
>>> I'd imaging that als-class devices are simply named als%u. Just like
>>> hwmon devices are named hwmon%u, input devices are names input%u and
>>> event%u, etc. I don't know of any class pushing the naming down to its
>>> drivers, do you? The only example I can remember are i2c drivers back
>>> in year 1999, when they were part of lm-sensors.I have personally put
>>> an end to this years ago.
>> This debate started in the als thread. Personally I couldn't care less
>> either way but it does need to be put to bed before that subsystem merges.
>> To my mind either approach is trivially handled in a userspace library
>> so it doesn't matter.  I don't suppose you can remember what the original
>> reasons for squashing this naming approach were?
> 
> Code duplication. Having the same unique-id handling code repeated in
> 50 drivers was no fun, as it did not add any value compared to a
> central handling.
Counter argument placed (cc'd Pavel and Corentin for this point)
is that having a generic name, e.g. hwmon0 and a name field in sysfs
is superfluous when we can combine the two.
> 
>>>> @@ -296,13 +333,13 @@ static ssize_t tsl2550_show_lux1_input(struct device *dev,
>>>>  	return ret;
>>>>  }
>>>>  
>>>> -static DEVICE_ATTR(lux1_input, S_IRUGO,
>>>> +static DEVICE_ATTR(illuminance, S_IRUGO,
>>>>  		   tsl2550_show_lux1_input, NULL);
>>> Question: if I have a light sensing chip with two sensors, how are we
>>> going to handle it? Will we register 2 als class devices? The initial
>>> naming convention had the advantage that you could have more than one
>>> sensor per device, but I don't know if it is desirable in practice.
>> This only becomes and issue if we have two sensors measuring illuminance
>> (or approximation of it).  The only two sensor chips I know of have one
>> broadband and one in the infrared tsl2561 and I think the isl chip with
>> a driver currently in misc.  The combination of these two are needed to
>> calculate illuminance.  Some of the original discussion went into how
>> to support separate access to the individual sensors.  Decision was to
>> leave that question until it becomes relevant.  Basically we would put
>> the current drivers in just supporting illuminance and see if anyone asked
>> for furthers support.  One tricky aspect is what the units should be for
>> particular frequency ranges. At least illuminance is cleanly defined
>> (even if chips are only fairly coarsely approximating it. 
> 
> Hmm, this isn't the point I was raising. I was thinking of a light
> sensor chip which would sense light in different locations. Think chip
> with remote sensors. This is done frequently for thermal sensors, so I
> guess it could be done for light sensors as well. A practical
> application could be sensing the ambient light on two sides of an
> object, so that you get the correct measurement regardless of the
> position.
> 
> I'm not saying such chips exist, I really don't know. I am just raising
> the question of how we'd handle them if they do. The current naming
> scheme implies that we'd need a separate als instance for each sensor,
> and I want you to be aware of this.
I agree with this being a possible issue. Zhang, what do you think to changing
the acpi driver to use luminance0 and documentation to match?  Seems like
a cost free way of avoiding possible problems down the line.
> 
>>>>  static struct attribute *tsl2550_attributes[] = {
>>>>  	&dev_attr_power_state.attr,
>>>>  	&dev_attr_operating_mode.attr,
>>>> -	&dev_attr_lux1_input.attr,
>>>> +	&dev_attr_illuminance.attr,
>>>>  	NULL
>>>>  };
>>>>  
>>>> @@ -350,6 +387,7 @@ static int __devinit tsl2550_probe(struct i2c_client *client,
>>>>  	struct i2c_adapter *adapter = to_i2c_adapter(client->dev.parent);
>>>>  	struct tsl2550_data *data;
>>>>  	int *opmode, err = 0;
>>>> +	char name[9];
>>>>  
>>>>  	if (!i2c_check_functionality(adapter, I2C_FUNC_SMBUS_WRITE_BYTE
>>>>  					    | I2C_FUNC_SMBUS_READ_BYTE_DATA)) {
>>>> @@ -387,15 +425,31 @@ static int __devinit tsl2550_probe(struct i2c_client *client,
>>>>  	if (err)
>>>>  		goto exit_kfree;
>>>>  
>>>> +	data->id = tsl2550_get_id();
>>>> +	if (data->id < 0) {
>>>> +		err = data->id;
>>>> +		goto exit_kfree;
>>>> +	}
>>>> +	sprintf(name, "tsl2550%d", data->id);
>>> Please no. For one thing you should always use snprintf and not sprintf
>>> when writing to such small buffers. It's way too easy to get things
>>> wrong otherwise. And you really want a separator between the chip name
>>> and the id, because "tsl25500" will be confusing as hell.
>> The length is fine with the restriction above, but I agree we need a separation
>> (hadn't really thought this through!).
> 
> The length is fine until someone needs more sensors, changes
> TSL2550_DEV_MAX to 16, and then the code crashes and nobody knows why.
> You can't assume that other developers won't touch your code. Thus it
> is much better to handle a given limitation in a single place. Here you
> can simply check the value returned by snprintf() and then you know if
> your name was correctly set.
> 
> Again, not that it matters, because the name buffer should simply be
> large enough that it always fits.
That's awfully big if we don't place some arbitrary limit on number of
devices. I'm happy to pick another one, but one is needed. Clearly the
code can be made more resilient to the sort of change you mention as well
and I'll do that if this isn't moved into the core.

Jonathan

Re: [PATCH] ALS: TSL2550 driver move from i2c/chips

[Jean Delvare]

On Mon, 12 Oct 2009 18:02:12 +0100, Jonathan Cameron wrote:
> Jean Delvare wrote:
> > The name size really isn't an issue. You won't notice 16 bytes instead
> > of 9. And it's not like we'll have millions of these devices.
>
> I agree, it's merely a question of picking some suitable limit. IDR's
> on a 64 bit box will do something in the ball park of 2e18 which might
> be an excessive limit ;) I'll leave this be for next version on basis

In all honesty, I don't expect als device names of ~30 chars to cause a
memory shortage to any machine any time soon. I'm not claiming that we
are likely to see machines with 2e18 light sensors any time soon
either. All I'm saying is that arbitrary limits should be avoided when
possible.

> (...)
> Counter argument placed (cc'd Pavel and Corentin for this point)
> is that having a generic name, e.g. hwmon0 and a name field in sysfs
> is superfluous when we can combine the two.

Counter counter argument is that this then makes it impossible to have
different subtypes and differentiate by name. See the input subsystem
for an example: they have input%d and event%d devices, for two
different purposes. It's very easy for both users and user-space
software to see what is what with this naming scheme. Other examples of
this include video4linux and sound classes. As a matter of fact,
looking at all classes that are in use on my machine, I can't see any
class which leaves device naming up to its implementing drivers (except
apparently mem and misc, but you'll have to agree that these are very
special classes.)

As a summary, I can't see any reason to come up with a new way to
handle class device names in the als class, when all other classes
around have already agreed on a standard approach.

> > (...)
> > The length is fine until someone needs more sensors, changes
> > TSL2550_DEV_MAX to 16, and then the code crashes and nobody knows why.
> > You can't assume that other developers won't touch your code. Thus it
> > is much better to handle a given limitation in a single place. Here you
> > can simply check the value returned by snprintf() and then you know if
> > your name was correctly set.
> > 
> > Again, not that it matters, because the name buffer should simply be
> > large enough that it always fits.
>
> That's awfully big if we don't place some arbitrary limit on number of

Where "awfully big" is like 30 bytes. Wow, I'm frightened :)

> devices. I'm happy to pick another one, but one is needed. Clearly the
> code can be made more resilient to the sort of change you mention as well
> and I'll do that if this isn't moved into the core.

Whatever limit you end up with, it should be set by the buffer size,
rather than a separate constant.

-- 
Jean Delvare

Re: [PATCH] ALS: TSL2550 driver move from i2c/chips

[Zhang Rui]

On Tue, 2009-10-13 at 01:02 +0800, Jonathan Cameron wrote:
> Jean Delvare wrote:
> > Hi Jonathan,
> > 
> > On Mon, 12 Oct 2009 15:19:07 +0100, Jonathan Cameron wrote:
> >>>> @@ -60,9 +65,41 @@ static const u8 TSL2550_MODE_RANGE[2] = {
> >>>>  };
> >>>>  
> >>>>  /*
> >>>> + * IDR to assign each registered device a unique id
> >>>> + */
> >>>> +static DEFINE_IDR(tsl2550_idr);
> >>>> +static DEFINE_SPINLOCK(tsl2550_idr_lock);
> >>>> +#define TSL2550_DEV_MAX 9
> >>> Such an arbitrary limit is simply not acceptable.
> >> Fair enough, but it is based on restricting the size
> >> of the char array needed for the name when registering
> >> with als.  Hence single digit decimal maximum.
> >> Do you suggest leaving it unrestricted (which makes code
> >> a little fiddly given variable size of max idr) or some other
> >> limit?
> > 
> > The name size really isn't an issue. You won't notice 16 bytes instead
> > of 9. And it's not like we'll have millions of these devices.
> I agree, it's merely a question of picking some suitable limit. IDR's
> on a 64 bit box will do something in the ball park of 2e18 which might
> be an excessive limit ;) I'll leave this be for next version on basis
> I'm in favour of moving this into the core and hence as you said this
> will be irrelevant here anyway.
> >>>> +static int tsl2550_get_id(void)
> >>>> +{
> >>>> +	int ret, val;
> >>>> +
> >>>> +idr_again:
> >>>> +	if (unlikely(idr_pre_get(&tsl2550_idr, GFP_KERNEL) == 0))
> >>>> +		return -ENOMEM;
> >>>> +	spin_lock(&tsl2550_idr_lock);
> >>>> +	ret = idr_get_new(&tsl2550_idr, NULL, &val);
> >>>> +	if (unlikely(ret == -EAGAIN))
> >>>> +		goto idr_again;
> >>>> +	else if (unlikely(ret))
> >>>> +		return ret;
> >>>> +	if (val > TSL2550_DEV_MAX)
> >>>> +		return -ENOMEM;
> >>>> +	return val;
> >>>> +}
> >>>> +
> >>>> +static void tsl2550_free_id(int val)
> >>>> +{
> >>>> +	spin_lock(&tsl2550_idr_lock);
> >>>> +	idr_remove(&tsl2550_idr, val);
> >>>> +	spin_unlock(&tsl2550_idr_lock);
> >>>> +}
> >>> Having to implement this in _every_ ALS driver sounds wrong and
> >>> painful. If uniqueness of any kind must be provided, it should be
> >>> handled by the ALS core and not by individual drivers, otherwise you
> >>> can be certain that at least one driver will get it wrong someday.
> >> I agree. The reason this originally occurred is that the acpi layer
> >> apparently doesn't allow for simultaneous probing of multiple drivers
> >> and hence can get away with a simple counter and no locking.
> >>
> >>> I'd imaging that als-class devices are simply named als%u. Just like
> >>> hwmon devices are named hwmon%u, input devices are names input%u and
> >>> event%u, etc. I don't know of any class pushing the naming down to its
> >>> drivers, do you? The only example I can remember are i2c drivers back
> >>> in year 1999, when they were part of lm-sensors.I have personally put
> >>> an end to this years ago.
> >> This debate started in the als thread. Personally I couldn't care less
> >> either way but it does need to be put to bed before that subsystem merges.
> >> To my mind either approach is trivially handled in a userspace library
> >> so it doesn't matter.  I don't suppose you can remember what the original
> >> reasons for squashing this naming approach were?
> > 
> > Code duplication. Having the same unique-id handling code repeated in
> > 50 drivers was no fun, as it did not add any value compared to a
> > central handling.
> Counter argument placed (cc'd Pavel and Corentin for this point)
> is that having a generic name, e.g. hwmon0 and a name field in sysfs
> is superfluous when we can combine the two.
> > 
> >>>> @@ -296,13 +333,13 @@ static ssize_t tsl2550_show_lux1_input(struct device *dev,
> >>>>  	return ret;
> >>>>  }
> >>>>  
> >>>> -static DEVICE_ATTR(lux1_input, S_IRUGO,
> >>>> +static DEVICE_ATTR(illuminance, S_IRUGO,
> >>>>  		   tsl2550_show_lux1_input, NULL);
> >>> Question: if I have a light sensing chip with two sensors, how are we
> >>> going to handle it? Will we register 2 als class devices? The initial
> >>> naming convention had the advantage that you could have more than one
> >>> sensor per device, but I don't know if it is desirable in practice.
> >> This only becomes and issue if we have two sensors measuring illuminance
> >> (or approximation of it).  The only two sensor chips I know of have one
> >> broadband and one in the infrared tsl2561 and I think the isl chip with
> >> a driver currently in misc.  The combination of these two are needed to
> >> calculate illuminance.  Some of the original discussion went into how
> >> to support separate access to the individual sensors.  Decision was to
> >> leave that question until it becomes relevant.  Basically we would put
> >> the current drivers in just supporting illuminance and see if anyone asked
> >> for furthers support.  One tricky aspect is what the units should be for
> >> particular frequency ranges. At least illuminance is cleanly defined
> >> (even if chips are only fairly coarsely approximating it. 
> > 
> > Hmm, this isn't the point I was raising. I was thinking of a light
> > sensor chip which would sense light in different locations. Think chip
> > with remote sensors. This is done frequently for thermal sensors, so I
> > guess it could be done for light sensors as well. A practical
> > application could be sensing the ambient light on two sides of an
> > object, so that you get the correct measurement regardless of the
> > position.
> > 
> > I'm not saying such chips exist, I really don't know. I am just raising
> > the question of how we'd handle them if they do. The current naming
> > scheme implies that we'd need a separate als instance for each sensor,
> > and I want you to be aware of this.
> I agree with this being a possible issue. Zhang, what do you think to changing
> the acpi driver to use luminance0 and documentation to match?  Seems like
> a cost free way of avoiding possible problems down the line.
> > 
I don't have any objections to this.
I think we should push the generic ALS patch upstream first and I'll
refresh the ACPI ALS patch later.

thanks,
rui

Re: [PATCH] ALS: TSL2550 driver move from i2c/chips

[Zhang Rui]

update Pavel's email address.

On Fri, 2009-10-16 at 09:37 +0800, Zhang Rui wrote:
> On Tue, 2009-10-13 at 01:02 +0800, Jonathan Cameron wrote:
> > Jean Delvare wrote:
> > > Hi Jonathan,
> > > 
> > > On Mon, 12 Oct 2009 15:19:07 +0100, Jonathan Cameron wrote:
> > >>>> @@ -60,9 +65,41 @@ static const u8 TSL2550_MODE_RANGE[2] = {
> > >>>>  };
> > >>>>  
> > >>>>  /*
> > >>>> + * IDR to assign each registered device a unique id
> > >>>> + */
> > >>>> +static DEFINE_IDR(tsl2550_idr);
> > >>>> +static DEFINE_SPINLOCK(tsl2550_idr_lock);
> > >>>> +#define TSL2550_DEV_MAX 9
> > >>> Such an arbitrary limit is simply not acceptable.
> > >> Fair enough, but it is based on restricting the size
> > >> of the char array needed for the name when registering
> > >> with als.  Hence single digit decimal maximum.
> > >> Do you suggest leaving it unrestricted (which makes code
> > >> a little fiddly given variable size of max idr) or some other
> > >> limit?
> > > 
> > > The name size really isn't an issue. You won't notice 16 bytes instead
> > > of 9. And it's not like we'll have millions of these devices.
> > I agree, it's merely a question of picking some suitable limit. IDR's
> > on a 64 bit box will do something in the ball park of 2e18 which might
> > be an excessive limit ;) I'll leave this be for next version on basis
> > I'm in favour of moving this into the core and hence as you said this
> > will be irrelevant here anyway.
> > >>>> +static int tsl2550_get_id(void)
> > >>>> +{
> > >>>> +	int ret, val;
> > >>>> +
> > >>>> +idr_again:
> > >>>> +	if (unlikely(idr_pre_get(&tsl2550_idr, GFP_KERNEL) == 0))
> > >>>> +		return -ENOMEM;
> > >>>> +	spin_lock(&tsl2550_idr_lock);
> > >>>> +	ret = idr_get_new(&tsl2550_idr, NULL, &val);
> > >>>> +	if (unlikely(ret == -EAGAIN))
> > >>>> +		goto idr_again;
> > >>>> +	else if (unlikely(ret))
> > >>>> +		return ret;
> > >>>> +	if (val > TSL2550_DEV_MAX)
> > >>>> +		return -ENOMEM;
> > >>>> +	return val;
> > >>>> +}
> > >>>> +
> > >>>> +static void tsl2550_free_id(int val)
> > >>>> +{
> > >>>> +	spin_lock(&tsl2550_idr_lock);
> > >>>> +	idr_remove(&tsl2550_idr, val);
> > >>>> +	spin_unlock(&tsl2550_idr_lock);
> > >>>> +}
> > >>> Having to implement this in _every_ ALS driver sounds wrong and
> > >>> painful. If uniqueness of any kind must be provided, it should be
> > >>> handled by the ALS core and not by individual drivers, otherwise you
> > >>> can be certain that at least one driver will get it wrong someday.
> > >> I agree. The reason this originally occurred is that the acpi layer
> > >> apparently doesn't allow for simultaneous probing of multiple drivers
> > >> and hence can get away with a simple counter and no locking.
> > >>
> > >>> I'd imaging that als-class devices are simply named als%u. Just like
> > >>> hwmon devices are named hwmon%u, input devices are names input%u and
> > >>> event%u, etc. I don't know of any class pushing the naming down to its
> > >>> drivers, do you? The only example I can remember are i2c drivers back
> > >>> in year 1999, when they were part of lm-sensors.I have personally put
> > >>> an end to this years ago.
> > >> This debate started in the als thread. Personally I couldn't care less
> > >> either way but it does need to be put to bed before that subsystem merges.
> > >> To my mind either approach is trivially handled in a userspace library
> > >> so it doesn't matter.  I don't suppose you can remember what the original
> > >> reasons for squashing this naming approach were?
> > > 
> > > Code duplication. Having the same unique-id handling code repeated in
> > > 50 drivers was no fun, as it did not add any value compared to a
> > > central handling.
> > Counter argument placed (cc'd Pavel and Corentin for this point)
> > is that having a generic name, e.g. hwmon0 and a name field in sysfs
> > is superfluous when we can combine the two.
> > > 
> > >>>> @@ -296,13 +333,13 @@ static ssize_t tsl2550_show_lux1_input(struct device *dev,
> > >>>>  	return ret;
> > >>>>  }
> > >>>>  
> > >>>> -static DEVICE_ATTR(lux1_input, S_IRUGO,
> > >>>> +static DEVICE_ATTR(illuminance, S_IRUGO,
> > >>>>  		   tsl2550_show_lux1_input, NULL);
> > >>> Question: if I have a light sensing chip with two sensors, how are we
> > >>> going to handle it? Will we register 2 als class devices? The initial
> > >>> naming convention had the advantage that you could have more than one
> > >>> sensor per device, but I don't know if it is desirable in practice.
> > >> This only becomes and issue if we have two sensors measuring illuminance
> > >> (or approximation of it).  The only two sensor chips I know of have one
> > >> broadband and one in the infrared tsl2561 and I think the isl chip with
> > >> a driver currently in misc.  The combination of these two are needed to
> > >> calculate illuminance.  Some of the original discussion went into how
> > >> to support separate access to the individual sensors.  Decision was to
> > >> leave that question until it becomes relevant.  Basically we would put
> > >> the current drivers in just supporting illuminance and see if anyone asked
> > >> for furthers support.  One tricky aspect is what the units should be for
> > >> particular frequency ranges. At least illuminance is cleanly defined
> > >> (even if chips are only fairly coarsely approximating it. 
> > > 
> > > Hmm, this isn't the point I was raising. I was thinking of a light
> > > sensor chip which would sense light in different locations. Think chip
> > > with remote sensors. This is done frequently for thermal sensors, so I
> > > guess it could be done for light sensors as well. A practical
> > > application could be sensing the ambient light on two sides of an
> > > object, so that you get the correct measurement regardless of the
> > > position.
> > > 
> > > I'm not saying such chips exist, I really don't know. I am just raising
> > > the question of how we'd handle them if they do. The current naming
> > > scheme implies that we'd need a separate als instance for each sensor,
> > > and I want you to be aware of this.
> > I agree with this being a possible issue. Zhang, what do you think to changing
> > the acpi driver to use luminance0 and documentation to match?  Seems like
> > a cost free way of avoiding possible problems down the line.
> > > 
> I don't have any objections to this.
> I think we should push the generic ALS patch upstream first and I'll
> refresh the ACPI ALS patch later.
> 
> thanks,
> rui