iio: jsa1212: Add JSA1212 proximity/ALS sensor
v3 cover iio: jsa1212: Add JSA1212 proximity/ALS sensor
Please let me know your review comments.
PATCH v3 changes:
1. Replaced status flags with bool values.
2. Added regmap suppport to avoid adding custom i2c register update routine.
3. Also addressed some minor comments from Jonathan Cameron
PATCH v2 changes:
1. Combined ALS/PXS enable/disable functions into a single routine.
2. Removed extra debug messages.
3. Also addressed review comments from Peter Meerwald
Kuppuswamy Sathyanarayanan (1):
iio: jsa1212: Add JSA1212 proximity/ALS sensor
drivers/iio/light/Kconfig | 10 + drivers/iio/light/Makefile | 1 + drivers/iio/light/jsa1212.c | 487 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 498 insertions(+) create mode 100644 drivers/iio/light/jsa1212.c
Signature · 1 line
1.7.9.5
PATCH v3 1/1 iio: jsa1212: Add JSA1212 proximity/ALS sensor
This patch adds a new driver for solteam opto JSA1212 proximity and ambient light sensor.
Basic details of the chip can be found here.
http://www.solteamopto.com.tw/detail.php?ms=3&po_unit=2&pt_unit=29&p_unit=120
--- drivers/iio/light/Kconfig | 10 + drivers/iio/light/Makefile | 1 + drivers/iio/light/jsa1212.c | 487 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 498 insertions(+) create mode 100644 drivers/iio/light/jsa1212.c
drivers/iio/light/Kconfig +10 -0
@@ -87,6 +87,16 @@ config HID_SENSOR_PROX To compile this driver as a module, choose M here: the module will be called hid-sensor-prox. +config JSA1212 + tristate "JSA1212 ALS and proximity sensor driver" + depends on I2C + help + Say Y here if you want to build a IIO driver for JSA1212 + proximity & ALS sensor device. + + To compile this driver as a module, choose M here: + the module will be called jsa1212. + config SENSORS_LM3533 tristate "LM3533 ambient light sensor" depends on MFD_LM3533
drivers/iio/light/Makefile +1 -0
@@ -10,6 +10,7 @@ obj-$(CONFIG_CM36651) += cm36651.o obj-$(CONFIG_GP2AP020A00F) += gp2ap020a00f.o obj-$(CONFIG_HID_SENSOR_ALS) += hid-sensor-als.o obj-$(CONFIG_HID_SENSOR_PROX) += hid-sensor-prox.o +obj-$(CONFIG_JSA1212) += jsa1212.o obj-$(CONFIG_SENSORS_LM3533) += lm3533-als.o obj-$(CONFIG_LTR501) += ltr501.o obj-$(CONFIG_SENSORS_TSL2563) += tsl2563.o
drivers/iio/light/jsa1212.c new file +487 -0
487 lines not on this page · raw message
Signature · 1 line
1.7.9.5
Jonathan Cameron
maintainerQuoted 9 lines · Kuppuswamy Sathyanarayanan
This patch adds a new driver for solteam opto JSA1212 proximity and ambient light sensor.
Basic details of the chip can be found here.
http://www.solteamopto.com.tw/detail.php?ms=3&po_unit=2&pt_unit=29&p_unit=120
Signed-off-by Kuppuswamy Sathyanarayanan <[email protected]>Signed-off-by Srinivas Pandruvada <[email protected]>
Mostly looking good. A few bits and bobs inline.
J
Quoted 64 lines
--- drivers/iio/light/Kconfig | 10 + drivers/iio/light/Makefile | 1 + drivers/iio/light/jsa1212.c | 487 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 498 insertions(+) create mode 100644 drivers/iio/light/jsa1212.cdrivers/iio/light/Kconfig +10
-0@@ -87,6 +87,16 @@ config HID_SENSOR_PROX To compile this driver as a module, choose M here: the module will be called hid-sensor-prox. +config JSA1212 + tristate "JSA1212 ALS and proximity sensor driver" + depends on I2C + help + Say Y here if you want to build a IIO driver for JSA1212 + proximity & ALS sensor device. + + To compile this driver as a module, choose M here: + the module will be called jsa1212. + config SENSORS_LM3533 tristate "LM3533 ambient light sensor" depends on MFD_LM3533drivers/iio/light/Makefile +1
-0@@ -10,6 +10,7 @@ obj-$(CONFIG_CM36651) += cm36651.o obj-$(CONFIG_GP2AP020A00F) += gp2ap020a00f.o obj-$(CONFIG_HID_SENSOR_ALS) += hid-sensor-als.o obj-$(CONFIG_HID_SENSOR_PROX) += hid-sensor-prox.o +obj-$(CONFIG_JSA1212) += jsa1212.o obj-$(CONFIG_SENSORS_LM3533) += lm3533-als.o obj-$(CONFIG_LTR501) += ltr501.o obj-$(CONFIG_SENSORS_TSL2563) += tsl2563.odrivers/iio/light/jsa1212.c new file +18
-0@@ -0,0 +1,487 @@ +/* +* JSA1212 Ambient Light & Proximity Sensor Driver +* +* Copyright (c) 2014, Intel Corporation. +* +* This program is free software; you can redistribute it and/or modify it +* under the terms and conditions of the GNU General Public License, +* version 2, as published by the Free Software Foundation. +* +* This program is distributed in the hope it will be useful, but WITHOUT +* ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or +* FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for +* more details. +*/ + +#include <linux/kernel.h> +#include <linux/slab.h> +#include <linux/module.h>
drivers/iio/light/jsa1212.c line 28
Quoted 10 lines
drivers/iio/light/jsa1212.c new file +10
-0+#include <linux/moduleparam.h> +#include <linux/delay.h> +#include <linux/i2c.h> +#include <linux/interrupt.h> +#include <linux/irq.h> +#include <linux/mutex.h> +#include <linux/slab.h> +#include <linux/acpi.h> +#include <linux/regmap.h> +#include <linux/gpio/consumer.h>
Why gpio?
drivers/iio/light/jsa1212.c line 31
Quoted 3 lines
drivers/iio/light/jsa1212.c new file +3
-0+#include <linux/iio/iio.h> +#include <linux/iio/sysfs.h> +#include <linux/iio/events.h>
you don't have any events so don't need htis header...
Quoted 7 lines
drivers/iio/light/jsa1212.c new file +7
-0+ +/* JSA1212 reg address */ +#define JSA1212_CONF_REG 0x01 +#define JSA1212_INT_REG 0x02 +#define JSA1212_PXS_LT_REG 0x03 +#define JSA1212_PXS_HT_REG 0x04 +#define JSA1212_ALS_TH1_REG 0x05
drivers/iio/light/jsa1212.c line 48
Quoted 10 lines
drivers/iio/light/jsa1212.c new file +10
-0+#define JSA1212_ALS_TH2_REG 0x06 +#define JSA1212_ALS_TH3_REG 0x07 +#define JSA1212_PXS_DATA_REG 0x08 +#define JSA1212_ALS_DT1_REG 0x09 +#define JSA1212_ALS_DT2_REG 0x0A +#define JSA1212_ALS_RNG_REG 0x0B +#define JSA1212_MAX_REG 0x0C + +/* JSA1212 reg defaults */ +#define JSA1212_CONF_REG_DEF 0x58
Interestingly this consists of exclusively bits not list below. If possible break it up into what it is actually doing...
drivers/iio/light/jsa1212.c line 49
Quoted 1 line
drivers/iio/light/jsa1212.c new file +1
-0+#define JSA1212_INT_REG_DEF 0x02
This one looks like it can be given in terms of what it is doing using the definitions below. Please do so.
drivers/iio/light/jsa1212.c line 55
Quoted 6 lines
drivers/iio/light/jsa1212.c new file +6
-0+#define JSA1212_PXS_LT_REG_DEF 0x00 +#define JSA1212_PXS_HT_REG_DEF 0xFF +#define JSA1212_ALS_TH1_REG_DEF 0x00 +#define JSA1212_ALS_TH2_REG_DEF 0xF0 +#define JSA1212_ALS_TH3_REG_DEF 0xFF +#define JSA1212_ALS_RNG_REG_DEF 0x00
There are a fair number of defines here that are unused and don't add anything it terms of documentation etc... Perhaps prune them back somewhat?
Quoted 124 lines
drivers/iio/light/jsa1212.c new file +124
-0+ +/* JSA1212 reg masks */ +#define JSA1212_CONF_MASK 0xFF +#define JSA1212_INT_MASK 0xFF +#define JSA1212_PXS_LT_MASK 0xFF +#define JSA1212_PXS_HT_MASK 0xFF +#define JSA1212_ALS_TH1_MASK 0xFF +#define JSA1212_ALS_TH2_LT_MASK 0x0F +#define JSA1212_ALS_TH2_HT_MASK 0xF0 +#define JSA1212_ALS_TH3_MASK 0xFF +#define JSA1212_PXS_DATA_MASK 0xFF +#define JSA1212_ALS_DATA_MASK 0xFFF +#define JSA1212_ALS_DT1_MASK 0xFF +#define JSA1212_ALS_DT2_MASK 0x0F +#define JSA1212_ALS_RNG_MASK 0x07 +#define JSA1212_REG_MASK 0xFF + +/* JSA1212 CONF REG bits */ +#define JSA1212_CONF_PXS_MASK 0x80 +#define JSA1212_CONF_PXS_ENABLE 0x80 +#define JSA1212_CONF_PXS_DISABLE 0x00 +#define JSA1212_CONF_ALS_MASK 0x04 +#define JSA1212_CONF_ALS_ENABLE 0x04 +#define JSA1212_CONF_ALS_DISABLE 0x00 + +/* JSA1212 INT REG bits */ +#define JSA1212_INT_CTRL_MASK 0x01 +#define JSA1212_INT_CTRL_EITHER 0x00 +#define JSA1212_INT_CTRL_BOTH 0x01 +#define JSA1212_INT_ALS_PRST_MASK 0x06 +#define JSA1212_INT_ALS_PRST_1CONV 0x00 +#define JSA1212_INT_ALS_PRST_4CONV 0x02 +#define JSA1212_INT_ALS_PRST_8CONV 0x04 +#define JSA1212_INT_ALS_PRST_16CONV 0x06 +#define JSA1212_INT_ALS_FLAG_MASK 0x08 +#define JSA1212_INT_ALS_FLAG_CLR 0x00 +#define JSA1212_INT_PXS_PRST_MASK 0x60 +#define JSA1212_INT_PXS_PRST_1CONV 0x00 +#define JSA1212_INT_PXS_PRST_4CONV 0x20 +#define JSA1212_INT_PXS_PRST_8CONV 0x40 +#define JSA1212_INT_PXS_PRST_16CONV 0x60 +#define JSA1212_INT_PXS_FLAG_MASK 0x80 +#define JSA1212_INT_PXS_FLAG_CLR 0x00 + +/* JSA1212 ALS RNG REG bits */ +#define JSA1212_ALS_RNG_0_2048 0x00 +#define JSA1212_ALS_RNG_0_1024 0x01 +#define JSA1212_ALS_RNG_0_512 0x02 +#define JSA1212_ALS_RNG_0_256 0x03 +#define JSA1212_ALS_RNG_0_128 0x04 + +/* JSA1212 INT threshold range */ +#define JSA1212_ALS_TH_MIN 0x0000 +#define JSA1212_ALS_TH_MAX 0x0FFF +#define JSA1212_PXS_TH_MIN 0x00 +#define JSA1212_PXS_TH_MAX 0xFF + +#define JSA1212_ALS_DELAY_MS 0xC8 +#define JSA1212_PXS_DELAY_MS 0x64 + +#define JSA1212_DRIVER_NAME "jsa1212" +#define JSA1212_REGMAP_NAME "jsa1212_regmap" + +enum jsa1212_op_mode { +JSA1212_OPMODE_ALS_EN , +JSA1212_OPMODE_PXS_EN , +}; + +struct jsa1212_data { +struct i2c_client *client; +struct mutex lock; +u8 als_rng_idx; +bool als_en;/* ALS enable status */ +bool pxs_en;/* proximity enable status */ +struct regmap *regmap; +}; + +/* ALS range idx to val mapping */ +static const int jsa1212_als_range_val[] = {2048 ,1024 ,512 ,256 ,128 , +128 ,128 ,128 }; + +/* Enables or disables ALS function based on status */ +static int jsa1212_als_enable (struct jsa1212_data *data,u8 status) +{ +int ret; + + ret =regmap_update_bits (data->regmap,JSA1212_CONF_REG , +JSA1212_CONF_ALS_MASK , + status); +if (ret <0 ) +return ret; + + data->als_en = status ? true : false; + +return ret; +} + +/* Enables or disables PXS function based on status */ +static int jsa1212_pxs_enable (struct jsa1212_data *data,u8 status) +{ +int ret; + + ret =regmap_update_bits (data->regmap,JSA1212_CONF_REG , +JSA1212_CONF_PXS_MASK , + status); +if (ret <0 ) +return ret; + + data->pxs_en = status ? true : false; + +return ret; +} + +static int jsa1212_read_als_data (struct jsa1212_data *data, +unsigned int *val) +{ +int ret; +u8 als_data[2 ]; + + ret =jsa1212_als_enable (data,JSA1212_CONF_ALS_ENABLE ); +if (ret <0 ) +return ret; + +/* Delay for data output */
drivers/iio/light/jsa1212.c line 189
Quoted 10 lines
drivers/iio/light/jsa1212.c new file +10
-0+msleep (JSA1212_ALS_DELAY_MS ); + +/* Read 12 bit data */ + ret =regmap_bulk_read (data->regmap,JSA1212_ALS_DT1_REG , als_data,2 ); +if (ret <0 ) { +dev_err (&data->client->dev,"als data read err\n" ); +return ret; + } + + *val = als_data[1 ] <<8 | als_data[0 ];
This looks like an endian conversion. If possible use one of the standard endian conversion functions. That way it is free if one isn't needed.
Quoted 210 lines
drivers/iio/light/jsa1212.c new file +210
-0+ + ret =jsa1212_als_enable (data,JSA1212_CONF_ALS_DISABLE ); +if (ret <0 ) +return ret; + +return 0 ; +} + +static int jsa1212_read_pxs_data (struct jsa1212_data *data, +unsigned int *val) +{ +int ret; +unsigned int pxs_data; + + ret =jsa1212_pxs_enable (data,JSA1212_CONF_PXS_ENABLE ); +if (ret <0 ) +return ret; + +/* Delay for data output */ +msleep (JSA1212_PXS_DELAY_MS ); + +/* Read out all data */ + ret =regmap_read (data->regmap,JSA1212_PXS_DATA_REG , &pxs_data); +if (ret <0 ) { +dev_err (&data->client->dev,"pxs data read err\n" ); +return ret; + } + + *val = pxs_data &JSA1212_PXS_DATA_MASK ; + + ret =jsa1212_pxs_enable (data,JSA1212_CONF_PXS_DISABLE ); +if (ret <0 ) +return ret; + +return 0 ; +} + +static int jsa1212_read_raw (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan, +int *val,int *val2,long mask) +{ +int ret; +struct jsa1212_data *data =iio_priv (indio_dev); + +switch (mask) { +case IIO_CHAN_INFO_RAW : +mutex_lock (&data->lock); +switch (chan->type) { +case IIO_LIGHT : + ret =jsa1212_read_als_data (data, val); +break ; +case IIO_PROXIMITY : + ret =jsa1212_read_pxs_data (data, val); +break ; +default : + ret = -EINVAL ; +break ; + } + ret = ret <0 ? ret :IIO_VAL_INT ; +mutex_unlock (&data->lock); +break ; +case IIO_CHAN_INFO_SCALE : +switch (chan->type) { +case IIO_LIGHT : + *val = jsa1212_als_range_val[data->als_rng_idx]; + *val2 =BIT (12 );/* Max 12 bit value */ + ret =IIO_VAL_FRACTIONAL ; +break ; +default : + ret = -EINVAL ; +break ; + } +break ; +default : + ret = -EINVAL ; +break ; + } + +return ret; +} + +static const struct iio_chan_spec jsa1212_channels[] = { + { + .type =IIO_LIGHT , + .info_mask_separate =BIT (IIO_CHAN_INFO_RAW ) | +BIT (IIO_CHAN_INFO_SCALE ), + }, + { + .type =IIO_PROXIMITY , + .info_mask_separate =BIT (IIO_CHAN_INFO_RAW ), + } +}; + +static const struct iio_info jsa1212_info = { + .driver_module =THIS_MODULE , + .read_raw = &jsa1212_read_raw, +}; + +static int jsa1212_chip_init (struct jsa1212_data *data) +{ +int ret; + + ret =regmap_write (data->regmap,JSA1212_CONF_REG , +JSA1212_CONF_REG_DEF ); +if (ret <0 ) +return ret; + + ret =regmap_write (data->regmap,JSA1212_INT_REG , +JSA1212_INT_REG_DEF ); +if (ret <0 ) +return ret; + + data->als_rng_idx =JSA1212_ALS_RNG_0_2048 ; + +return 0 ; +} + +static bool jsa1212_is_volatile_reg (struct device *dev,unsigned int reg) +{ +switch (reg) { +case JSA1212_PXS_DATA_REG : +case JSA1212_ALS_DT1_REG : +case JSA1212_ALS_DT2_REG : +case JSA1212_INT_REG : +return true; +default : +return false; + } +} + +static struct regmap_config jsa1212_regmap_config = { + .name =JSA1212_REGMAP_NAME , + .reg_bits =8 , + .val_bits =8 , + .max_register =JSA1212_MAX_REG , + .cache_type =REGCACHE_RBTREE , + .volatile_reg = jsa1212_is_volatile_reg, +}; + +static int jsa1212_probe (struct i2c_client *client, +const struct i2c_device_id *id) +{ +struct jsa1212_data *jsa1212_data; +struct iio_dev *indio_dev; +struct regmap *regmap; +int ret; + +if (!i2c_check_functionality (client->adapter,I2C_FUNC_SMBUS_BYTE_DATA )) +return -ENODEV ; + + indio_dev =devm_iio_device_alloc (&client->dev,sizeof (*jsa1212_data)); +if (!indio_dev) +return -ENOMEM ; + + regmap =devm_regmap_init_i2c (client, &jsa1212_regmap_config); +if (IS_ERR (regmap)) { +dev_err (&client->dev,"Regmap initialization failed.\n" ); +return PTR_ERR (regmap); + } + + jsa1212_data =iio_priv (indio_dev); + +i2c_set_clientdata (client, indio_dev); + jsa1212_data->client = client; + jsa1212_data->regmap = regmap; + +mutex_init (&jsa1212_data->lock); + + ret =jsa1212_chip_init (jsa1212_data); +if (ret <0 ) +return ret; + + indio_dev->dev.parent = &client->dev; + indio_dev->channels = jsa1212_channels; + indio_dev->num_channels =ARRAY_SIZE (jsa1212_channels); + indio_dev->name =JSA1212_DRIVER_NAME ; + indio_dev->modes =INDIO_DIRECT_MODE ; + + indio_dev->info = &jsa1212_info; + + ret =iio_device_register (indio_dev); +if (ret <0 ) { +dev_err (&client->dev,"%s: register device failed\n" , __func__); +return ret; + } + +return 0 ; +} + +/* power off the device */ +static int jsa1212_power_off (struct jsa1212_data *data) +{ +int ret; + +mutex_lock (&data->lock); + + ret =regmap_update_bits (data->regmap,JSA1212_CONF_REG , +JSA1212_CONF_ALS_MASK | +JSA1212_CONF_PXS_MASK , +JSA1212_CONF_ALS_DISABLE | +JSA1212_CONF_PXS_DISABLE ); + +if (ret <0 ) +dev_err (&data->client->dev,"power off cmd failed\n" ); + +mutex_unlock (&data->lock); + +return ret; +} +
drivers/iio/light/jsa1212.c line 409
Quoted 10 lines
drivers/iio/light/jsa1212.c new file +10
-0+ +static int jsa1212_remove (struct i2c_client *client) +{ +struct iio_dev *indio_dev =i2c_get_clientdata (client); +struct jsa1212_data *data =iio_priv (indio_dev); +int ret; + + ret =jsa1212_power_off (data); + +iio_device_unregister (indio_dev);
I'd definitely expect the userspace interfaces to be removed before the power off! Which is to say the two lines above are in the wrong order.
Quoted 17 lines
drivers/iio/light/jsa1212.c new file +17
-0+ +return ret; +} + +#ifdef CONFIG_PM_SLEEP +static int jsa1212_suspend (struct device *dev) +{ +struct jsa1212_data *data; + + data =iio_priv (i2c_get_clientdata (to_i2c_client (dev))); + +return jsa1212_power_off (data); +} + +static int jsa1212_resume (struct device *dev) +{ +int ret =0 ;
drivers/iio/light/jsa1212.c line 436
Quoted 10 lines
drivers/iio/light/jsa1212.c new file +10
-0+struct jsa1212_data *data; + + data =iio_priv (i2c_get_clientdata (to_i2c_client (dev))); + +mutex_lock (&data->lock); + +if (data->als_en) { + ret =jsa1212_als_enable (data,JSA1212_CONF_ALS_ENABLE ); +if (ret <0 ) { +dev_err (dev,"als resume failed\n" );
When there is a mutex to unlock, it's often cleaner to have a single point of exit and use a goto.
Quoted 52 lines
drivers/iio/light/jsa1212.c new file +51
-0+mutex_unlock (&data->lock); + return ret; + } + } + +if (data->pxs_en) { + ret =jsa1212_pxs_enable (data,JSA1212_CONF_PXS_ENABLE ); +if (ret <0 ) +dev_err (dev,"pxs resume failed\n" ); + } + +mutex_unlock (&data->lock); + +return ret; +} + +static SIMPLE_DEV_PM_OPS(jsa1212_pm_ops, jsa1212_suspend, jsa1212_resume); + +#define JSA1212_PM_OPS (&jsa1212_pm_ops) +#else +#define JSA1212_PM_OPS NULL +#endif + +static const struct acpi_device_id jsa1212_acpi_match[] = { + {"JSA1212" ,0 }, + { }, +}; +MODULE_DEVICE_TABLE (acpi, jsa1212_acpi_match); + +static const struct i2c_device_id jsa1212_id[] = { + {JSA1212_DRIVER_NAME ,0 }, + { } +}; +MODULE_DEVICE_TABLE (i2c, jsa1212_id); + +static struct i2c_driver jsa1212_driver = { + .driver = { + .name =JSA1212_DRIVER_NAME , + .pm =JSA1212_PM_OPS , + .owner =THIS_MODULE , + .acpi_match_table =ACPI_PTR (jsa1212_acpi_match), + }, + .probe = jsa1212_probe, + .remove = jsa1212_remove, + .id_table = jsa1212_id, +}; +module_i2c_driver (jsa1212_driver); + +MODULE_AUTHOR ("Sathya Kuppuswamy <[email protected]>" ); +MODULE_DESCRIPTION ("JSA1212 proximity/ambient light sensor driver" ); +MODULE_LICENSE ("GPL v2" );