ALS: TSL2550 driver move from i2c/chips
PATCH 1/1 ALS: TSL2550 driver move from i2c/chips
---
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(-)drivers/als/Kconfig +14 -0
@@ -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
drivers/als/Makefile +2 -0
@@ -3,3 +3,5 @@ # obj-$(CONFIG_ALS) += als_sys.o + +obj-$(CONFIG_ALS_TSL2550) += tsl2550.o
drivers/als/tsl2550.c renamed from drivers/i2c/chips/tsl2550.c +66 -7
138 lines not on this page · raw message
Signature · 1 line
1.6.3.3
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.
Quoted 9 lines
struct
tsl2550_data{drivers/als/tsl2550.c renamed from drivers/i2c/chips/tsl2550.c +2
-0+struct device *classdev;struct i2c_client *client;struct mutex update_lock; +unsigned int id; unsigned int power_state : 1; unsigned int operating_mode : 1;
....
drivers/als/tsl2550.c static int __devinit tsl2550_probe(struct i2c_client *client, · line 432
Quoted 7 lines
drivers/als/tsl2550.c renamed from drivers/i2c/chips/tsl2550.c +5
-0+ data->id =tsl2550_get_id (); +if (data->id <0 ) { + err = data->id; + goto exit_kfree; + }
Jonathan
Gah, I forgot the I2C dependency as well.
Sorry for the silly errors.
Quoted 214 lines
Signed-off-by Jonathan Cameron <[email protected]>---
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(-)drivers/als/Kconfig +14
-0@@ -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 #ALSdrivers/als/Makefile +2
-0@@ -3,3 +3,5 @@ # obj-$(CONFIG_ALS) += als_sys.o + +obj-$(CONFIG_ALS_TSL2550) += tsl2550.odrivers/als/tsl2550.c renamed from drivers/i2c/chips/tsl2550.c +66
-7@@ -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 ; }
Hi Jonathan,
Quoted 6 lines · Jonathan Cameron
Signed-off-by Jonathan Cameron <jic23-KWPb1pKIrIJaa/[email protected]>---
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.
Quoted 5 lines
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.
Quoted 3 lines
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?
Quoted 1 line
Does anyone want a patch without using Git's move functionality?
Yes please!
Quoted 10 lines
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(-)
Signature · 1 line
Jean Delvare
Hi Jean,
Quoted 21 lines · Jonathan Cameron
Quoted 5 lines
Signed-off-by Jonathan Cameron <jic23-KWPb1pKIrIJaa/[email protected]>---
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.
Quoted 5 lines
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.
Quoted 8 lines
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?
Quoted 5 lines
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?
Quoted 10 lines
Quoted 4 lines
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...
Quoted 4 lines
Quoted 2 lines
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!)
Quoted 12 lines
Quoted 11 lines
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(-)
Quoted 31 lines · Jonathan Cameron
Hi Jean,
Quoted 21 lines · Jonathan Cameron
Quoted 5 lines
Signed-off-by Jonathan Cameron <jic23-KWPb1pKIrIJaa/[email protected]>---
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.
Quoted 5 lines
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.
Quoted 8 lines
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.
Quoted 3 lines
That sounds like a sensible approach. For now I guess a sysfs parameter like
illuminance_range_maxwould do the job and we can add a min for any device that features an offset?
Yes, this sounds sensible.
Quoted 7 lines
Quoted 4 lines
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.
Quoted 16 lines
Quoted 9 lines
Quoted 3 lines
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.
Quoted 6 lines
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/levelWe 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.
Signature · 1 line
Jean Delvare
Quoted 14 lines · Jean Delvare
Quoted 13 lines · Jonathan Cameron
Quoted 12 lines
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.
Quoted 5 lines
Quoted 3 lines
That sounds like a sensible approach. For now I guess a sysfs parameter like
illuminance_range_maxwould 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.
Signature · 1 line
Jean Delvare
Quoted 4 lines · Jonathan Cameron
drivers/als/Kconfig | 14 ++++++
drivers/als/Makefile | 2 +
drivers/{i2c/chips => als}/tsl2550.c | 73 ++++++++++++++++++++++++++++++---
3 files changed, 82 insertions(+), 7 deletions(-)Review:
Quoted 5 lines
drivers/als/Kconfig +0
-0
drivers/als/Kconfig menuconfig ALS · line 16
Quoted 9 lines
drivers/als/Kconfig +6
-0@@ -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.
Quoted 27 lines
drivers/als/Kconfig +8
-0+ 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 #ALSdrivers/als/Makefile +2
-0@@ -3,3 +3,5 @@ # obj-$(CONFIG_ALS) += als_sys.o + +obj-$(CONFIG_ALS_TSL2550) += tsl2550.odrivers/als/tsl2550.c renamed from drivers/i2c/chips/tsl2550.c +0
-0
drivers/als/tsl2550.c line 32
Quoted 10 lines
drivers/als/tsl2550.c renamed from drivers/i2c/chips/tsl2550.c +4
-1@@ -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"?
Quoted 15 lines
drivers/als/tsl2550.c renamed from drivers/i2c/chips/tsl2550.c +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 ;
drivers/als/tsl2550.c static const u8 TSL2550_MODE_RANGE[2] = { · line 72
Quoted 8 lines
drivers/als/tsl2550.c renamed from drivers/i2c/chips/tsl2550.c +5
-0@@ -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.
Quoted 19 lines
drivers/als/tsl2550.c renamed from drivers/i2c/chips/tsl2550.c +16
-0+ +/* * 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 )
drivers/als/tsl2550.c static const u8 TSL2550_MODE_RANGE[2] = { · line 101
Quoted 10 lines
drivers/als/tsl2550.c renamed from drivers/i2c/chips/tsl2550.c +10
-0+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.
drivers/als/tsl2550.c static ssize_t tsl2550_show_lux1_input(struct device *dev, · line 337
Quoted 11 lines
drivers/als/tsl2550.c renamed from drivers/i2c/chips/tsl2550.c +2
-1+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.
Quoted 18 lines
drivers/als/tsl2550.c renamed from drivers/i2c/chips/tsl2550.c +2
-1static 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 )) {
drivers/als/tsl2550.c static int __devinit tsl2550_probe(struct i2c_client *client, · line 433
Quoted 9 lines
drivers/als/tsl2550.c renamed from drivers/i2c/chips/tsl2550.c +6
-0@@ -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.
Quoted 44 lines
drivers/als/tsl2550.c renamed from drivers/i2c/chips/tsl2550.c +19
-4/* 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 ; }
Signature · 1 line
Jean Delvare
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.
Quoted 2 lines
"2.0"?
True, that would be consistent.
Quoted 18 lines
Quoted 18 lines
/*
* Definesdrivers/als/tsl2550.c renamed from drivers/i2c/chips/tsl2550.c +2
-0@@ -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] = { };
Quoted 8 lines
drivers/als/tsl2550.c static const u8 TSL2550_MODE_RANGE[2] = { · line 72
Quoted 6 lines
drivers/als/tsl2550.c renamed from drivers/i2c/chips/tsl2550.c +5
-0/* +* IDR to assign each registered device a unique id +*/ +static DEFINE_IDR (tsl2550_idr); +static DEFINE_SPINLOCK (tsl2550_idr_lock); +#define TSL2550_DEV_MAX 9Such 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?
Quoted 26 lines
Quoted 26 lines
drivers/als/tsl2550.c renamed from drivers/i2c/chips/tsl2550.c +23
-0+ +/* * 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);
Quoted 8 lines
drivers/als/tsl2550.c static const u8 TSL2550_MODE_RANGE[2] = { · line 101
Quoted 3 lines
drivers/als/tsl2550.c renamed from drivers/i2c/chips/tsl2550.c +3
-0+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.
Quoted 7 lines
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?
Quoted 9 lines
Quoted 9 lines
drivers/als/tsl2550.c renamed from drivers/i2c/chips/tsl2550.c +1
-0+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; }
Quoted 8 lines
drivers/als/tsl2550.c static ssize_t tsl2550_show_lux1_input(struct device *dev, · line 337
Quoted 3 lines
drivers/als/tsl2550.c renamed from drivers/i2c/chips/tsl2550.c +1
-1-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.
Quoted 22 lines
Quoted 22 lines
drivers/als/tsl2550.c renamed from drivers/i2c/chips/tsl2550.c +2
-1static 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;
Quoted 11 lines
drivers/als/tsl2550.c static int __devinit tsl2550_probe(struct i2c_client *client, · line 433
Quoted 6 lines
drivers/als/tsl2550.c renamed from drivers/i2c/chips/tsl2550.c +6
-0+ 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!).
Quoted 3 lines
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.
Quoted 47 lines
Quoted 45 lines
drivers/als/tsl2550.c renamed from drivers/i2c/chips/tsl2550.c +19
-4/* 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 ; }
Hi Jonathan,
Quoted 12 lines · Jonathan Cameron
Quoted 11 lines
drivers/als/tsl2550.c static const u8 TSL2550_MODE_RANGE[2] = { · line 72
Quoted 9 lines
drivers/als/tsl2550.c renamed from drivers/i2c/chips/tsl2550.c +5
-0@@ -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 9Such an arbitrary limit is simply not acceptable.
Quoted 6 lines
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.
Quoted 41 lines
Quoted 21 lines
Quoted 21 lines
drivers/als/tsl2550.c renamed from drivers/i2c/chips/tsl2550.c +21
-0+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);Quoted 8 lines
drivers/als/tsl2550.c static const u8 TSL2550_MODE_RANGE[2] = { · line 101
Quoted 3 lines
drivers/als/tsl2550.c renamed from drivers/i2c/chips/tsl2550.c +3
-0+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.
Quoted 6 lines
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.
Quoted 5 lines
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.
Quoted 13 lines
Quoted 4 lines
Quoted 4 lines
drivers/als/tsl2550.c renamed from drivers/i2c/chips/tsl2550.c +0
-0@@ -296,13 +333,13 @@ static ssize_t tsl2550_show_lux1_input(struct device *dev,return ret; }Quoted 8 lines
drivers/als/tsl2550.c static ssize_t tsl2550_show_lux1_input(struct device *dev, · line 337
Quoted 3 lines
drivers/als/tsl2550.c renamed from drivers/i2c/chips/tsl2550.c +1
-1-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.
Quoted 11 lines
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.
Quoted 32 lines
Quoted 20 lines
Quoted 20 lines
drivers/als/tsl2550.c renamed from drivers/i2c/chips/tsl2550.c +2
-1static 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;Quoted 11 lines
drivers/als/tsl2550.c static int __devinit tsl2550_probe(struct i2c_client *client, · line 433
Quoted 6 lines
drivers/als/tsl2550.c renamed from drivers/i2c/chips/tsl2550.c +6
-0+ 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.
Quoted 2 lines
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.
Signature · 1 line
Jean Delvare
Quoted 20 lines · Jean Delvare
Hi Jonathan,
Quoted 10 lines · Jonathan Cameron
Quoted 10 lines
drivers/als/tsl2550.c static const u8 TSL2550_MODE_RANGE[2] = { · line 72
Quoted 9 lines
drivers/als/tsl2550.c renamed from drivers/i2c/chips/tsl2550.c +5
-0@@ -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 9Such an arbitrary limit is simply not acceptable.
Quoted 6 lines
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?
Quoted 2 lines
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.
Quoted 44 lines
Quoted 38 lines
Quoted 20 lines
Quoted 20 lines
drivers/als/tsl2550.c renamed from drivers/i2c/chips/tsl2550.c +20
-0+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) +{Quoted 8 lines
drivers/als/tsl2550.c static const u8 TSL2550_MODE_RANGE[2] = { · line 101
Quoted 4 lines
drivers/als/tsl2550.c renamed from drivers/i2c/chips/tsl2550.c +4
-0+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.
Quoted 6 lines
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.
Quoted 5 lines
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?
Quoted 3 lines
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.
Quoted 32 lines
Quoted 12 lines
Quoted 5 lines
Quoted 5 lines
drivers/als/tsl2550.c renamed from drivers/i2c/chips/tsl2550.c +0
-0@@ -296,13 +333,13 @@ static ssize_t tsl2550_show_lux1_input(struct device *dev,return ret; }Quoted 7 lines
drivers/als/tsl2550.c static ssize_t tsl2550_show_lux1_input(struct device *dev, · line 337
Quoted 3 lines
drivers/als/tsl2550.c renamed from drivers/i2c/chips/tsl2550.c +1
-1-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.
Quoted 11 lines
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.
Quoted 4 lines
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.
Quoted 41 lines
Quoted 31 lines
Quoted 21 lines
Quoted 21 lines
drivers/als/tsl2550.c renamed from drivers/i2c/chips/tsl2550.c +2
-1static 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;Quoted 10 lines
drivers/als/tsl2550.c static int __devinit tsl2550_probe(struct i2c_client *client, · line 433
Quoted 6 lines
drivers/als/tsl2550.c renamed from drivers/i2c/chips/tsl2550.c +6
-0+ 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.
Quoted 2 lines
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_MAXto 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 bysnprintf()and then you know if your name was correctly set.
Quoted 2 lines
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
Quoted 7 lines · Jonathan Cameron
Quoted 2 lines · Jean Delvare
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.
Quoted 4 lines
(...)
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.
Quoted 8 lines
Quoted 8 lines
(...)
The length is fine until someone needs more sensors, changesTSL2550_DEV_MAXto 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 bysnprintf()and then you know if your name was correctly set.
Quoted 4 lines
Quoted 2 lines
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 :)
Quoted 3 lines
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.
Signature · 1 line
Jean Delvare
Quoted 110 lines · Jonathan Cameron
Quoted 20 lines · Jean Delvare
Hi Jonathan,
Quoted 10 lines · Jonathan Cameron
Quoted 10 lines
drivers/als/tsl2550.c static const u8 TSL2550_MODE_RANGE[2] = { · line 72
Quoted 9 lines
drivers/als/tsl2550.c renamed from drivers/i2c/chips/tsl2550.c +5
-0@@ -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 9Such an arbitrary limit is simply not acceptable.
Quoted 6 lines
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?Quoted 2 lines
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.
Quoted 44 lines
Quoted 38 lines
Quoted 20 lines
Quoted 20 lines
drivers/als/tsl2550.c renamed from drivers/i2c/chips/tsl2550.c +20
-0+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) +{Quoted 8 lines
drivers/als/tsl2550.c static const u8 TSL2550_MODE_RANGE[2] = { · line 101
Quoted 4 lines
drivers/als/tsl2550.c renamed from drivers/i2c/chips/tsl2550.c +4
-0+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.
Quoted 6 lines
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.
Quoted 5 lines
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?
Quoted 3 lines
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.
Quoted 32 lines
Quoted 12 lines
Quoted 5 lines
Quoted 5 lines
drivers/als/tsl2550.c renamed from drivers/i2c/chips/tsl2550.c +0
-0@@ -296,13 +333,13 @@ static ssize_t tsl2550_show_lux1_input(struct device *dev,return ret; }Quoted 7 lines
drivers/als/tsl2550.c static ssize_t tsl2550_show_lux1_input(struct device *dev, · line 337
Quoted 3 lines
drivers/als/tsl2550.c renamed from drivers/i2c/chips/tsl2550.c +1
-1-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.
Quoted 11 lines
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.
Quoted 8 lines
Quoted 4 lines
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.
Quoted 1 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
update Pavel's email address.
Quoted 125 lines · Zhang Rui
Quoted 110 lines · Jonathan Cameron
Quoted 20 lines · Jean Delvare
Hi Jonathan,
Quoted 10 lines · Jonathan Cameron
Quoted 10 lines
drivers/als/tsl2550.c static const u8 TSL2550_MODE_RANGE[2] = { · line 72
Quoted 9 lines
drivers/als/tsl2550.c renamed from drivers/i2c/chips/tsl2550.c +5
-0@@ -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 9Such an arbitrary limit is simply not acceptable.
Quoted 6 lines
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?Quoted 2 lines
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.
Quoted 44 lines
Quoted 38 lines
Quoted 20 lines
Quoted 20 lines
drivers/als/tsl2550.c renamed from drivers/i2c/chips/tsl2550.c +20
-0+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) +{Quoted 8 lines
drivers/als/tsl2550.c static const u8 TSL2550_MODE_RANGE[2] = { · line 101
Quoted 4 lines
drivers/als/tsl2550.c renamed from drivers/i2c/chips/tsl2550.c +4
-0+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.
Quoted 6 lines
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.
Quoted 5 lines
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?
Quoted 3 lines
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.
Quoted 32 lines
Quoted 12 lines
Quoted 5 lines
Quoted 5 lines
drivers/als/tsl2550.c renamed from drivers/i2c/chips/tsl2550.c +0
-0@@ -296,13 +333,13 @@ static ssize_t tsl2550_show_lux1_input(struct device *dev,return ret; }Quoted 7 lines
drivers/als/tsl2550.c static ssize_t tsl2550_show_lux1_input(struct device *dev, · line 337
Quoted 3 lines
drivers/als/tsl2550.c renamed from drivers/i2c/chips/tsl2550.c +1
-1-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.
Quoted 11 lines
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.
Quoted 8 lines
Quoted 4 lines
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.
Quoted 1 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