* [PATCH v2] iio: light: Add support for ROHM RPR0521 sensor
@ 2015-05-28 10:52 Daniel Baluta
2015-05-28 11:35 ` Peter Meerwald
0 siblings, 1 reply; 9+ messages in thread
From: Daniel Baluta @ 2015-05-28 10:52 UTC (permalink / raw)
To: jic23; +Cc: pmeerw, knaack.h, lars, linux-kernel, linux-iio, daniel.baluta
This patch adds support for ROHM RPR0521 ambient light and proximity
sensor. It offers raw readings for intensity and proximity.
Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
---
Changes since v1: (after Jonathan's review)
* add static const to some structures
* make some local functions static
* added clarification for set_power_state function.
drivers/iio/light/Kconfig | 11 +
drivers/iio/light/Makefile | 1 +
drivers/iio/light/rpr0521.c | 596 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 608 insertions(+)
create mode 100644 drivers/iio/light/rpr0521.c
diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index e6198b7..cbc4677 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -168,6 +168,17 @@ config JSA1212
To compile this driver as a module, choose M here:
the module will be called jsa1212.
+config RPR0521
+ tristate "ROHM RPR0521 ALS and proximity sensor driver"
+ depends on I2C
+ select REGMAP_I2C
+ help
+ Say Y here if you want to build support for ROHM's RPR0521
+ ambient light and proximity sensor device.
+
+ To compile this driver as a module, choose M here:
+ the module will be called rpr0521.
+
config SENSORS_LM3533
tristate "LM3533 ambient light sensor"
depends on MFD_LM3533
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index e2d50fd..66fc2fd 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -17,6 +17,7 @@ obj-$(CONFIG_HID_SENSOR_ALS) += hid-sensor-als.o
obj-$(CONFIG_HID_SENSOR_PROX) += hid-sensor-prox.o
obj-$(CONFIG_ISL29125) += isl29125.o
obj-$(CONFIG_JSA1212) += jsa1212.o
+obj-$(CONFIG_RPR0521) += rpr0521.o
obj-$(CONFIG_SENSORS_LM3533) += lm3533-als.o
obj-$(CONFIG_LTR501) += ltr501.o
obj-$(CONFIG_SENSORS_TSL2563) += tsl2563.o
diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
new file mode 100644
index 0000000..45d4df5
--- /dev/null
+++ b/drivers/iio/light/rpr0521.c
@@ -0,0 +1,596 @@
+/*
+ * RPR-0521 ROHM Ambient Light and Proximity Sensor
+ *
+ * Copyright (c) 2015, Intel Corporation.
+ *
+ * This file is subject to the terms and conditions of version 2 of
+ * the GNU General Public License. See the file COPYING in the main
+ * directory of this archive for more details.
+ *
+ * IIO driver for RPR-0521RS (7-bit I2C slave address 0x38).
+ *
+ * TODO: illuminance channel, PM support, buffer
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/i2c.h>
+#include <linux/regmap.h>
+#include <linux/delay.h>
+#include <linux/acpi.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/pm_runtime.h>
+
+#define RPR0521_REG_SYSTEM_CTRL 0x40
+#define RPR0521_REG_MODE_CTRL 0x41
+#define RPR0521_REG_ALS_CTRL 0x42
+#define RPR0521_REG_PXS_CTRL 0x43
+#define RPR0521_REG_PXS_DATA 0x44 /* 16-bit, little endian */
+#define RPR0521_REG_ALS_DATA0 0x46 /* 16-bit, little endian */
+#define RPR0521_REG_ALS_DATA1 0x48 /* 16-bit, little endian */
+#define RPR0521_REG_ID 0x92
+
+#define RPR0521_MODE_ALS_MASK BIT(7)
+#define RPR0521_MODE_PXS_MASK BIT(6)
+#define RPR0521_MODE_MEAS_TIME_MASK GENMASK(3, 0)
+#define RPR0521_ALS_DATA0_GAIN_MASK (BIT(4) | BIT(5))
+#define RPR0521_ALS_DATA0_GAIN_SHIFT 4
+#define RPR0521_ALS_DATA1_GAIN_MASK (BIT(2) | BIT(3))
+#define RPR0521_ALS_DATA1_GAIN_SHIFT 2
+#define RPR0521_PXS_GAIN_MASK (BIT(4) | BIT(5))
+#define RPR0521_PXS_GAIN_SHIFT 4
+
+#define RPR0521_MODE_ALS_ENABLE BIT(7)
+#define RPR0521_MODE_ALS_DISABLE 0x00
+#define RPR0521_MODE_PXS_ENABLE BIT(6)
+#define RPR0521_MODE_PXS_DISABLE 0x00
+
+#define RPR0521_PXS_MAX_VAL (BIT(13) - 1)
+#define RPR0521_MANUFACT_ID 0xE0
+#define RPR0521_DEFAULT_MEAS_TIME 0x06 /* ALS - 100ms, PXS - 100ms */
+
+#define RPR0521_DRV_NAME "RPR0521"
+#define RPR0521_REGMAP_NAME "rpr0521_regmap"
+
+#define RPR0521_SLEEP_DELAY_MS 2000
+
+static const int rpr0521_als_gain[4] = {1, 2, 64, 128};
+static const int rpr0521_pxs_gain[3] = {1, 2, 4};
+
+enum rpr0521_channel {
+ RPR0521_CHAN_ALS_DATA0,
+ RPR0521_CHAN_ALS_DATA1,
+ RPR0521_CHAN_PXS,
+};
+
+static const int rpr0521_data_reg[] = {
+ [RPR0521_CHAN_ALS_DATA0] = RPR0521_REG_ALS_DATA0,
+ [RPR0521_CHAN_ALS_DATA1] = RPR0521_REG_ALS_DATA1,
+ [RPR0521_CHAN_PXS] = RPR0521_REG_PXS_DATA,
+};
+
+static const struct rpr0521_gain_info {
+ u8 reg;
+ u8 mask;
+ u8 shift;
+ int *gain;
+ int size;
+} rpr0521_gain[] = {
+ [RPR0521_CHAN_ALS_DATA0] = {
+ .reg = RPR0521_REG_ALS_CTRL,
+ .mask = RPR0521_ALS_DATA0_GAIN_MASK,
+ .shift = RPR0521_ALS_DATA0_GAIN_SHIFT,
+ .gain = (int *)rpr0521_als_gain,
+ .size = ARRAY_SIZE(rpr0521_als_gain),
+ },
+ [RPR0521_CHAN_ALS_DATA1] = {
+ .reg = RPR0521_REG_ALS_CTRL,
+ .mask = RPR0521_ALS_DATA1_GAIN_MASK,
+ .shift = RPR0521_ALS_DATA1_GAIN_SHIFT,
+ .gain = (int *)rpr0521_als_gain,
+ .size = ARRAY_SIZE(rpr0521_als_gain),
+ },
+ [RPR0521_CHAN_PXS] = {
+ .reg = RPR0521_REG_PXS_CTRL,
+ .mask = RPR0521_PXS_GAIN_MASK,
+ .shift = RPR0521_PXS_GAIN_SHIFT,
+ .gain = (int *)rpr0521_pxs_gain,
+ .size = ARRAY_SIZE(rpr0521_pxs_gain),
+ },
+};
+
+struct rpr0521_data {
+ struct i2c_client *client;
+
+ /* protect device params updates (e.g state, gain) */
+ struct mutex lock;
+
+ /* device active status */
+ bool als_dev_en;
+ bool pxs_dev_en;
+
+ /* optimize runtime pm ops - enable device only if needed */
+ bool als_ps_need_en;
+ bool pxs_ps_need_en;
+
+ struct regmap *regmap;
+};
+
+static const struct iio_chan_spec rpr0521_channels[] = {
+ {
+ .type = IIO_INTENSITY,
+ .modified = 1,
+ .address = RPR0521_CHAN_ALS_DATA0,
+ .channel2 = IIO_MOD_LIGHT_BOTH,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+ BIT(IIO_CHAN_INFO_CALIBSCALE),
+ },
+ {
+ .type = IIO_INTENSITY,
+ .modified = 1,
+ .address = RPR0521_CHAN_ALS_DATA1,
+ .channel2 = IIO_MOD_LIGHT_IR,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+ BIT(IIO_CHAN_INFO_CALIBSCALE),
+ },
+ {
+ .type = IIO_PROXIMITY,
+ .address = RPR0521_CHAN_PXS,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+ BIT(IIO_CHAN_INFO_CALIBSCALE),
+ }
+};
+
+static int rpr0521_als_enable(struct rpr0521_data *data, u8 status)
+{
+ int ret;
+
+ ret = regmap_update_bits(data->regmap, RPR0521_REG_MODE_CTRL,
+ RPR0521_MODE_ALS_MASK,
+ status);
+ if (ret < 0)
+ return ret;
+
+ data->als_dev_en = true;
+
+ return 0;
+}
+
+static int rpr0521_pxs_enable(struct rpr0521_data *data, u8 status)
+{
+ int ret;
+
+ ret = regmap_update_bits(data->regmap, RPR0521_REG_MODE_CTRL,
+ RPR0521_MODE_PXS_MASK,
+ status);
+ if (ret < 0)
+ return ret;
+
+ data->pxs_dev_en = true;
+
+ return 0;
+}
+
+/**
+ * rpr0521_set_power_state - handles runtime PM state and sensors enabled status
+ *
+ * @data: rpr0521 device private data
+ * @on: state to be set for devices in @device_mask
+ * @device_mask: bitmask specifying for which device we need to update @on state
+ *
+ * We rely on rpr0521_runtime_resume to enable our @device_mask devices, but
+ * if (for example) PXS was enabled (pxs_dev_en = true) by a previous call to
+ * rpr0521_runtime_resume and we want to enable ALS we MUST set ALS enable
+ * bit of RPR0521_REG_MODE_CTRL here because rpr0521_runtime_resume will not
+ * be called twice.
+ */
+static int rpr0521_set_power_state(struct rpr0521_data *data, bool on,
+ u8 device_mask)
+{
+#ifdef CONFIG_PM
+ int ret;
+ u8 update_mask = 0;
+
+ if (device_mask & RPR0521_MODE_ALS_MASK) {
+ if (on && !data->als_ps_need_en && data->pxs_dev_en)
+ update_mask |= RPR0521_MODE_ALS_MASK;
+ else
+ data->als_ps_need_en = on;
+ }
+
+ if (device_mask & RPR0521_MODE_PXS_MASK) {
+ if (on && !data->pxs_ps_need_en && data->als_dev_en)
+ update_mask |= RPR0521_MODE_PXS_MASK;
+ else
+ data->pxs_ps_need_en = on;
+ }
+
+ if (update_mask) {
+ ret = regmap_update_bits(data->regmap, RPR0521_REG_MODE_CTRL,
+ update_mask, update_mask);
+ if (ret < 0)
+ return ret;
+ }
+
+ if (on) {
+ ret = pm_runtime_get_sync(&data->client->dev);
+ } else {
+ pm_runtime_mark_last_busy(&data->client->dev);
+ ret = pm_runtime_put_autosuspend(&data->client->dev);
+ }
+ if (ret < 0) {
+ dev_err(&data->client->dev,
+ "Failed: rpr0521_set_power_state for %d, ret %d\n",
+ on, ret);
+ if (on)
+ pm_runtime_put_noidle(&data->client->dev);
+
+ return ret;
+ }
+#endif
+ return 0;
+}
+
+static int rpr0521_get_gain_index(int *gainv, int size, int gain)
+{
+ int i;
+
+ for (i = 0; i < size; i++)
+ if (gain == gainv[i])
+ return i;
+
+ return -EINVAL;
+}
+
+static int rpr0521_get_gain(struct rpr0521_data *data, int chan, int *val)
+{
+ int ret, reg, idx;
+
+ ret = regmap_read(data->regmap, rpr0521_gain[chan].reg, ®);
+ if (ret < 0)
+ return ret;
+
+ idx = (rpr0521_gain[chan].mask & reg) >> rpr0521_gain[chan].shift;
+ *val = rpr0521_gain[chan].gain[idx];
+
+ return 0;
+}
+
+static int rpr0521_set_gain(struct rpr0521_data *data, int chan, int val)
+{
+ int idx;
+
+ idx = rpr0521_get_gain_index(rpr0521_gain[chan].gain,
+ rpr0521_gain[chan].size, val);
+ if (idx < 0)
+ return idx;
+
+ return regmap_update_bits(data->regmap, rpr0521_gain[chan].reg,
+ rpr0521_gain[chan].mask,
+ idx << rpr0521_gain[chan].shift);
+}
+
+static int rpr0521_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan, int *val,
+ int *val2, long mask)
+{
+ struct rpr0521_data *data = iio_priv(indio_dev);
+ int ret;
+ u8 device_mask;
+ __le16 raw_data;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ switch (chan->type) {
+ case IIO_INTENSITY:
+ device_mask = RPR0521_MODE_ALS_MASK;
+ break;
+ case IIO_PROXIMITY:
+ device_mask = RPR0521_MODE_PXS_MASK;
+ break;
+ default:
+ return -EINVAL;
+ }
+ mutex_lock(&data->lock);
+ ret = rpr0521_set_power_state(data, true, device_mask);
+ if (ret < 0) {
+ mutex_unlock(&data->lock);
+ return ret;
+ }
+
+ ret = regmap_bulk_read(data->regmap,
+ rpr0521_data_reg[chan->address],
+ &raw_data, 2);
+ if (ret < 0) {
+ rpr0521_set_power_state(data, false, device_mask);
+ mutex_unlock(&data->lock);
+ return ret;
+ }
+
+ ret = rpr0521_set_power_state(data, false, device_mask);
+ mutex_unlock(&data->lock);
+ if (ret < 0)
+ return ret;
+
+ *val = le16_to_cpu(raw_data);
+ /*
+ * proximity uses 12 bits, with bits 7:4 of PXS MSB DATA
+ * being always zero. Also, proximity MUST be exposed as
+ * a distance with lower values for closer objects.
+ */
+ if (chan->type == IIO_PROXIMITY)
+ *val = RPR0521_PXS_MAX_VAL - *val;
+
+ return IIO_VAL_INT;
+ case IIO_CHAN_INFO_CALIBSCALE:
+ mutex_lock(&data->lock);
+ ret = rpr0521_get_gain(data, chan->address, val);
+ mutex_unlock(&data->lock);
+ if (ret < 0)
+ return ret;
+
+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int rpr0521_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan, int val,
+ int val2, long mask)
+{
+ struct rpr0521_data *data = iio_priv(indio_dev);
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_CALIBSCALE:
+ mutex_lock(&data->lock);
+ ret = rpr0521_set_gain(data, chan->address, val);
+ mutex_unlock(&data->lock);
+
+ return ret;
+ default:
+ return -EINVAL;
+ }
+}
+
+static IIO_CONST_ATTR(in_intensity_calibscale_available, "1 2 64 128");
+static IIO_CONST_ATTR(in_proximity_calibscale_available, "1 2 4");
+
+static struct attribute *rpr0521_attributes[] = {
+ &iio_const_attr_in_intensity_calibscale_available.dev_attr.attr,
+ &iio_const_attr_in_proximity_calibscale_available.dev_attr.attr,
+ NULL
+};
+
+static const struct attribute_group rpr0521_attribute_group = {
+ .attrs = rpr0521_attributes,
+};
+
+static const struct iio_info rpr0521_info = {
+ .driver_module = THIS_MODULE,
+ .read_raw = rpr0521_read_raw,
+ .write_raw = rpr0521_write_raw,
+ .attrs = &rpr0521_attribute_group,
+};
+
+static int rpr0521_init(struct rpr0521_data *data)
+{
+ int ret;
+ int id;
+
+ ret = regmap_read(data->regmap, RPR0521_REG_ID, &id);
+ if (ret < 0) {
+ dev_err(&data->client->dev, "Failed to read REG_ID register\n");
+ return ret;
+ }
+
+ if (id != RPR0521_MANUFACT_ID) {
+ dev_err(&data->client->dev, "Wrong id, got %x expected %x\n",
+ id, RPR0521_MANUFACT_ID);
+ return -ENODEV;
+ }
+
+ /* set default measurement time - 100 ms for both ALS and PS */
+ ret = regmap_update_bits(data->regmap, RPR0521_REG_MODE_CTRL,
+ RPR0521_MODE_MEAS_TIME_MASK,
+ RPR0521_DEFAULT_MEAS_TIME);
+ if (ret) {
+ pr_err("regmap_update bits returned %d\n", ret);
+ return ret;
+ }
+
+ ret = rpr0521_als_enable(data, RPR0521_MODE_ALS_ENABLE);
+ if (ret < 0)
+ return ret;
+ ret = rpr0521_pxs_enable(data, RPR0521_MODE_PXS_ENABLE);
+ if (ret < 0)
+ return ret;
+
+ return 0;
+}
+
+static int rpr0521_poweroff(struct rpr0521_data *data)
+{
+ int ret;
+
+ ret = regmap_update_bits(data->regmap, RPR0521_REG_MODE_CTRL,
+ RPR0521_MODE_ALS_MASK |
+ RPR0521_MODE_PXS_MASK,
+ 0x00);
+ if (ret < 0)
+ return ret;
+
+ data->als_dev_en = false;
+ data->pxs_dev_en = false;
+
+ return 0;
+}
+
+static bool rpr0521_is_volatile_reg(struct device *dev, unsigned int reg)
+{
+ switch (reg) {
+ case RPR0521_REG_MODE_CTRL:
+ case RPR0521_REG_ALS_CTRL:
+ case RPR0521_REG_PXS_CTRL:
+ return false;
+ default:
+ return true;
+ }
+}
+
+static const struct regmap_config rpr0521_regmap_config = {
+ .name = RPR0521_REGMAP_NAME,
+
+ .reg_bits = 8,
+ .val_bits = 8,
+
+ .max_register = RPR0521_REG_ID,
+ .cache_type = REGCACHE_RBTREE,
+ .volatile_reg = rpr0521_is_volatile_reg,
+};
+
+static int rpr0521_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct rpr0521_data *data;
+ struct iio_dev *indio_dev;
+ struct regmap *regmap;
+ int ret;
+
+ indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ regmap = devm_regmap_init_i2c(client, &rpr0521_regmap_config);
+ if (IS_ERR(regmap)) {
+ dev_err(&client->dev, "Regmap init failed!\n");
+ return PTR_ERR(regmap);
+ }
+
+ data = iio_priv(indio_dev);
+ i2c_set_clientdata(client, indio_dev);
+ data->client = client;
+ data->regmap = regmap;
+
+ mutex_init(&data->lock);
+
+ indio_dev->dev.parent = &client->dev;
+ indio_dev->info = &rpr0521_info;
+ indio_dev->name = RPR0521_DRV_NAME;
+ indio_dev->channels = rpr0521_channels;
+ indio_dev->num_channels = ARRAY_SIZE(rpr0521_channels);
+ indio_dev->modes = INDIO_DIRECT_MODE;
+
+ ret = rpr0521_init(data);
+ if (ret < 0) {
+ dev_err(&client->dev, "rpr0521 chip init failed\n");
+ return ret;
+ }
+ ret = iio_device_register(indio_dev);
+ if (ret < 0)
+ return ret;
+
+ ret = pm_runtime_set_active(&client->dev);
+ if (ret < 0)
+ goto err_iio_unregister;
+
+ pm_runtime_enable(&client->dev);
+ pm_runtime_set_autosuspend_delay(&client->dev, RPR0521_SLEEP_DELAY_MS);
+ pm_runtime_use_autosuspend(&client->dev);
+
+ return 0;
+err_iio_unregister:
+ iio_device_unregister(indio_dev);
+ return ret;
+}
+
+static int rpr0521_remove(struct i2c_client *client)
+{
+ struct iio_dev *indio_dev = i2c_get_clientdata(client);
+
+ pm_runtime_disable(&client->dev);
+ pm_runtime_set_suspended(&client->dev);
+ pm_runtime_put_noidle(&client->dev);
+
+ iio_device_unregister(indio_dev);
+ rpr0521_poweroff(iio_priv(indio_dev));
+
+ return 0;
+}
+
+#ifdef CONFIG_PM
+static int rpr0521_runtime_suspend(struct device *dev)
+{
+ struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+ struct rpr0521_data *data = iio_priv(indio_dev);
+ int ret;
+
+ /* disable channels and sets {als,pxs}_dev_en to false */
+ mutex_lock(&data->lock);
+ ret = rpr0521_poweroff(data);
+ mutex_unlock(&data->lock);
+
+ return ret;
+}
+
+static int rpr0521_runtime_resume(struct device *dev)
+{
+ struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+ struct rpr0521_data *data = iio_priv(indio_dev);
+ int ret;
+
+ if (data->als_ps_need_en) {
+ ret = rpr0521_als_enable(data, RPR0521_MODE_ALS_ENABLE);
+ if (ret < 0)
+ return ret;
+ data->als_ps_need_en = false;
+ }
+
+ if (data->pxs_ps_need_en) {
+ ret = rpr0521_pxs_enable(data, RPR0521_MODE_PXS_ENABLE);
+ if (ret < 0)
+ return ret;
+ data->pxs_ps_need_en = false;
+ }
+
+ return 0;
+}
+#endif
+
+static const struct dev_pm_ops rpr0521_pm_ops = {
+ SET_RUNTIME_PM_OPS(rpr0521_runtime_suspend,
+ rpr0521_runtime_resume, NULL)
+};
+
+static const struct acpi_device_id rpr0521_acpi_match[] = {
+ {"RPR0521", 0},
+ { }
+};
+MODULE_DEVICE_TABLE(acpi, rpr0521_acpi_match);
+
+static const struct i2c_device_id rpr0521_id[] = {
+ {"rpr0521", 0},
+ { }
+};
+
+MODULE_DEVICE_TABLE(i2c, rpr0521_id);
+
+static struct i2c_driver rpr0521_driver = {
+ .driver = {
+ .name = RPR0521_DRV_NAME,
+ .pm = &rpr0521_pm_ops,
+ .acpi_match_table = ACPI_PTR(rpr0521_acpi_match),
+ },
+ .probe = rpr0521_probe,
+ .remove = rpr0521_remove,
+ .id_table = rpr0521_id,
+};
+
+module_i2c_driver(rpr0521_driver);
+
+MODULE_AUTHOR("Daniel Baluta <daniel.baluta@intel.com>");
+MODULE_DESCRIPTION("RPR0521 ROHM Ambient Light and Proximity Sensor driver");
+MODULE_LICENSE("GPL v2");
--
1.9.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] iio: light: Add support for ROHM RPR0521 sensor
2015-05-28 10:52 [PATCH v2] iio: light: Add support for ROHM RPR0521 sensor Daniel Baluta
@ 2015-05-28 11:35 ` Peter Meerwald
2015-05-28 12:49 ` Daniel Baluta
0 siblings, 1 reply; 9+ messages in thread
From: Peter Meerwald @ 2015-05-28 11:35 UTC (permalink / raw)
To: Daniel Baluta; +Cc: jic23, knaack.h, lars, linux-kernel, linux-iio
On Thu, 28 May 2015, Daniel Baluta wrote:
> This patch adds support for ROHM RPR0521 ambient light and proximity
> sensor. It offers raw readings for intensity and proximity.
comments below
> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
> ---
> Changes since v1: (after Jonathan's review)
> * add static const to some structures
> * make some local functions static
> * added clarification for set_power_state function.
>
> drivers/iio/light/Kconfig | 11 +
> drivers/iio/light/Makefile | 1 +
> drivers/iio/light/rpr0521.c | 596 ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 608 insertions(+)
> create mode 100644 drivers/iio/light/rpr0521.c
>
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index e6198b7..cbc4677 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -168,6 +168,17 @@ config JSA1212
> To compile this driver as a module, choose M here:
> the module will be called jsa1212.
>
> +config RPR0521
> + tristate "ROHM RPR0521 ALS and proximity sensor driver"
> + depends on I2C
> + select REGMAP_I2C
> + help
> + Say Y here if you want to build support for ROHM's RPR0521
> + ambient light and proximity sensor device.
> +
> + To compile this driver as a module, choose M here:
> + the module will be called rpr0521.
> +
> config SENSORS_LM3533
> tristate "LM3533 ambient light sensor"
> depends on MFD_LM3533
> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> index e2d50fd..66fc2fd 100644
> --- a/drivers/iio/light/Makefile
> +++ b/drivers/iio/light/Makefile
> @@ -17,6 +17,7 @@ obj-$(CONFIG_HID_SENSOR_ALS) += hid-sensor-als.o
> obj-$(CONFIG_HID_SENSOR_PROX) += hid-sensor-prox.o
> obj-$(CONFIG_ISL29125) += isl29125.o
> obj-$(CONFIG_JSA1212) += jsa1212.o
> +obj-$(CONFIG_RPR0521) += rpr0521.o
alphabetic order
> obj-$(CONFIG_SENSORS_LM3533) += lm3533-als.o
> obj-$(CONFIG_LTR501) += ltr501.o
> obj-$(CONFIG_SENSORS_TSL2563) += tsl2563.o
> diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
> new file mode 100644
> index 0000000..45d4df5
> --- /dev/null
> +++ b/drivers/iio/light/rpr0521.c
> @@ -0,0 +1,596 @@
> +/*
> + * RPR-0521 ROHM Ambient Light and Proximity Sensor
> + *
> + * Copyright (c) 2015, Intel Corporation.
> + *
> + * This file is subject to the terms and conditions of version 2 of
> + * the GNU General Public License. See the file COPYING in the main
> + * directory of this archive for more details.
> + *
> + * IIO driver for RPR-0521RS (7-bit I2C slave address 0x38).
> + *
> + * TODO: illuminance channel, PM support, buffer
PM seems to be supported?
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/i2c.h>
> +#include <linux/regmap.h>
> +#include <linux/delay.h>
> +#include <linux/acpi.h>
> +
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/pm_runtime.h>
> +
> +#define RPR0521_REG_SYSTEM_CTRL 0x40
> +#define RPR0521_REG_MODE_CTRL 0x41
> +#define RPR0521_REG_ALS_CTRL 0x42
> +#define RPR0521_REG_PXS_CTRL 0x43
> +#define RPR0521_REG_PXS_DATA 0x44 /* 16-bit, little endian */
> +#define RPR0521_REG_ALS_DATA0 0x46 /* 16-bit, little endian */
> +#define RPR0521_REG_ALS_DATA1 0x48 /* 16-bit, little endian */
> +#define RPR0521_REG_ID 0x92
> +
> +#define RPR0521_MODE_ALS_MASK BIT(7)
> +#define RPR0521_MODE_PXS_MASK BIT(6)
> +#define RPR0521_MODE_MEAS_TIME_MASK GENMASK(3, 0)
> +#define RPR0521_ALS_DATA0_GAIN_MASK (BIT(4) | BIT(5))
> +#define RPR0521_ALS_DATA0_GAIN_SHIFT 4
> +#define RPR0521_ALS_DATA1_GAIN_MASK (BIT(2) | BIT(3))
> +#define RPR0521_ALS_DATA1_GAIN_SHIFT 2
> +#define RPR0521_PXS_GAIN_MASK (BIT(4) | BIT(5))
> +#define RPR0521_PXS_GAIN_SHIFT 4
> +
> +#define RPR0521_MODE_ALS_ENABLE BIT(7)
> +#define RPR0521_MODE_ALS_DISABLE 0x00
> +#define RPR0521_MODE_PXS_ENABLE BIT(6)
> +#define RPR0521_MODE_PXS_DISABLE 0x00
_DISABLE is not used
> +
> +#define RPR0521_PXS_MAX_VAL (BIT(13) - 1)
> +#define RPR0521_MANUFACT_ID 0xE0
> +#define RPR0521_DEFAULT_MEAS_TIME 0x06 /* ALS - 100ms, PXS - 100ms */
> +
> +#define RPR0521_DRV_NAME "RPR0521"
> +#define RPR0521_REGMAP_NAME "rpr0521_regmap"
> +
> +#define RPR0521_SLEEP_DELAY_MS 2000
> +
> +static const int rpr0521_als_gain[4] = {1, 2, 64, 128};
> +static const int rpr0521_pxs_gain[3] = {1, 2, 4};
> +
> +enum rpr0521_channel {
> + RPR0521_CHAN_ALS_DATA0,
> + RPR0521_CHAN_ALS_DATA1,
> + RPR0521_CHAN_PXS,
> +};
> +
> +static const int rpr0521_data_reg[] = {
> + [RPR0521_CHAN_ALS_DATA0] = RPR0521_REG_ALS_DATA0,
> + [RPR0521_CHAN_ALS_DATA1] = RPR0521_REG_ALS_DATA1,
> + [RPR0521_CHAN_PXS] = RPR0521_REG_PXS_DATA,
> +};
> +
> +static const struct rpr0521_gain_info {
> + u8 reg;
> + u8 mask;
> + u8 shift;
> + int *gain;
why not const int *?
> + int size;
> +} rpr0521_gain[] = {
> + [RPR0521_CHAN_ALS_DATA0] = {
> + .reg = RPR0521_REG_ALS_CTRL,
> + .mask = RPR0521_ALS_DATA0_GAIN_MASK,
> + .shift = RPR0521_ALS_DATA0_GAIN_SHIFT,
> + .gain = (int *)rpr0521_als_gain,
> + .size = ARRAY_SIZE(rpr0521_als_gain),
> + },
> + [RPR0521_CHAN_ALS_DATA1] = {
> + .reg = RPR0521_REG_ALS_CTRL,
> + .mask = RPR0521_ALS_DATA1_GAIN_MASK,
> + .shift = RPR0521_ALS_DATA1_GAIN_SHIFT,
> + .gain = (int *)rpr0521_als_gain,
> + .size = ARRAY_SIZE(rpr0521_als_gain),
> + },
> + [RPR0521_CHAN_PXS] = {
> + .reg = RPR0521_REG_PXS_CTRL,
> + .mask = RPR0521_PXS_GAIN_MASK,
> + .shift = RPR0521_PXS_GAIN_SHIFT,
> + .gain = (int *)rpr0521_pxs_gain,
> + .size = ARRAY_SIZE(rpr0521_pxs_gain),
> + },
> +};
> +
> +struct rpr0521_data {
> + struct i2c_client *client;
> +
> + /* protect device params updates (e.g state, gain) */
> + struct mutex lock;
> +
> + /* device active status */
> + bool als_dev_en;
> + bool pxs_dev_en;
> +
> + /* optimize runtime pm ops - enable device only if needed */
> + bool als_ps_need_en;
> + bool pxs_ps_need_en;
> +
> + struct regmap *regmap;
> +};
> +
> +static const struct iio_chan_spec rpr0521_channels[] = {
> + {
> + .type = IIO_INTENSITY,
> + .modified = 1,
> + .address = RPR0521_CHAN_ALS_DATA0,
> + .channel2 = IIO_MOD_LIGHT_BOTH,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_CALIBSCALE),
why CALIBSCALE and not SCALE?
> + },
> + {
> + .type = IIO_INTENSITY,
> + .modified = 1,
> + .address = RPR0521_CHAN_ALS_DATA1,
> + .channel2 = IIO_MOD_LIGHT_IR,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_CALIBSCALE),
> + },
> + {
> + .type = IIO_PROXIMITY,
> + .address = RPR0521_CHAN_PXS,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_CALIBSCALE),
> + }
> +};
> +
> +static int rpr0521_als_enable(struct rpr0521_data *data, u8 status)
> +{
> + int ret;
> +
> + ret = regmap_update_bits(data->regmap, RPR0521_REG_MODE_CTRL,
> + RPR0521_MODE_ALS_MASK,
> + status);
> + if (ret < 0)
> + return ret;
> +
> + data->als_dev_en = true;
> +
> + return 0;
> +}
> +
> +static int rpr0521_pxs_enable(struct rpr0521_data *data, u8 status)
> +{
> + int ret;
> +
> + ret = regmap_update_bits(data->regmap, RPR0521_REG_MODE_CTRL,
> + RPR0521_MODE_PXS_MASK,
> + status);
status would fit on the line above?
> + if (ret < 0)
> + return ret;
> +
> + data->pxs_dev_en = true;
> +
> + return 0;
> +}
> +
> +/**
> + * rpr0521_set_power_state - handles runtime PM state and sensors enabled status
> + *
> + * @data: rpr0521 device private data
> + * @on: state to be set for devices in @device_mask
> + * @device_mask: bitmask specifying for which device we need to update @on state
> + *
> + * We rely on rpr0521_runtime_resume to enable our @device_mask devices, but
> + * if (for example) PXS was enabled (pxs_dev_en = true) by a previous call to
> + * rpr0521_runtime_resume and we want to enable ALS we MUST set ALS enable
> + * bit of RPR0521_REG_MODE_CTRL here because rpr0521_runtime_resume will not
> + * be called twice.
> + */
> +static int rpr0521_set_power_state(struct rpr0521_data *data, bool on,
> + u8 device_mask)
> +{
> +#ifdef CONFIG_PM
> + int ret;
> + u8 update_mask = 0;
> +
> + if (device_mask & RPR0521_MODE_ALS_MASK) {
> + if (on && !data->als_ps_need_en && data->pxs_dev_en)
> + update_mask |= RPR0521_MODE_ALS_MASK;
> + else
> + data->als_ps_need_en = on;
> + }
> +
> + if (device_mask & RPR0521_MODE_PXS_MASK) {
> + if (on && !data->pxs_ps_need_en && data->als_dev_en)
> + update_mask |= RPR0521_MODE_PXS_MASK;
> + else
> + data->pxs_ps_need_en = on;
> + }
> +
> + if (update_mask) {
> + ret = regmap_update_bits(data->regmap, RPR0521_REG_MODE_CTRL,
> + update_mask, update_mask);
> + if (ret < 0)
> + return ret;
> + }
> +
> + if (on) {
> + ret = pm_runtime_get_sync(&data->client->dev);
> + } else {
> + pm_runtime_mark_last_busy(&data->client->dev);
> + ret = pm_runtime_put_autosuspend(&data->client->dev);
> + }
> + if (ret < 0) {
> + dev_err(&data->client->dev,
> + "Failed: rpr0521_set_power_state for %d, ret %d\n",
> + on, ret);
> + if (on)
> + pm_runtime_put_noidle(&data->client->dev);
> +
> + return ret;
> + }
> +#endif
> + return 0;
> +}
> +
> +static int rpr0521_get_gain_index(int *gainv, int size, int gain)
> +{
> + int i;
> +
> + for (i = 0; i < size; i++)
> + if (gain == gainv[i])
> + return i;
> +
> + return -EINVAL;
> +}
> +
> +static int rpr0521_get_gain(struct rpr0521_data *data, int chan, int *val)
> +{
> + int ret, reg, idx;
> +
> + ret = regmap_read(data->regmap, rpr0521_gain[chan].reg, ®);
> + if (ret < 0)
> + return ret;
> +
> + idx = (rpr0521_gain[chan].mask & reg) >> rpr0521_gain[chan].shift;
> + *val = rpr0521_gain[chan].gain[idx];
> +
> + return 0;
> +}
> +
> +static int rpr0521_set_gain(struct rpr0521_data *data, int chan, int val)
> +{
> + int idx;
> +
> + idx = rpr0521_get_gain_index(rpr0521_gain[chan].gain,
> + rpr0521_gain[chan].size, val);
> + if (idx < 0)
> + return idx;
> +
> + return regmap_update_bits(data->regmap, rpr0521_gain[chan].reg,
> + rpr0521_gain[chan].mask,
> + idx << rpr0521_gain[chan].shift);
> +}
> +
> +static int rpr0521_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int *val,
> + int *val2, long mask)
> +{
> + struct rpr0521_data *data = iio_priv(indio_dev);
> + int ret;
> + u8 device_mask;
> + __le16 raw_data;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + switch (chan->type) {
could put the mask into _data_reg to avoid the switch
> + case IIO_INTENSITY:
> + device_mask = RPR0521_MODE_ALS_MASK;
> + break;
> + case IIO_PROXIMITY:
> + device_mask = RPR0521_MODE_PXS_MASK;
> + break;
> + default:
> + return -EINVAL;
> + }
> + mutex_lock(&data->lock);
> + ret = rpr0521_set_power_state(data, true, device_mask);
> + if (ret < 0) {
> + mutex_unlock(&data->lock);
> + return ret;
> + }
> +
> + ret = regmap_bulk_read(data->regmap,
> + rpr0521_data_reg[chan->address],
> + &raw_data, 2);
> + if (ret < 0) {
> + rpr0521_set_power_state(data, false, device_mask);
> + mutex_unlock(&data->lock);
> + return ret;
> + }
> +
> + ret = rpr0521_set_power_state(data, false, device_mask);
> + mutex_unlock(&data->lock);
> + if (ret < 0)
> + return ret;
> +
> + *val = le16_to_cpu(raw_data);
> + /*
> + * proximity uses 12 bits, with bits 7:4 of PXS MSB DATA
> + * being always zero. Also, proximity MUST be exposed as
> + * a distance with lower values for closer objects.
this matter hasn't been settled, all other proximity sensor drivers do it
the other way around
> + */
> + if (chan->type == IIO_PROXIMITY)
> + *val = RPR0521_PXS_MAX_VAL - *val;
really this should be _PROCESSED, not _RAW?
how to handle it for buffered reads?
> +
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_CALIBSCALE:
> + mutex_lock(&data->lock);
> + ret = rpr0521_get_gain(data, chan->address, val);
> + mutex_unlock(&data->lock);
> + if (ret < 0)
> + return ret;
> +
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int rpr0521_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int val,
> + int val2, long mask)
> +{
> + struct rpr0521_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_CALIBSCALE:
> + mutex_lock(&data->lock);
> + ret = rpr0521_set_gain(data, chan->address, val);
> + mutex_unlock(&data->lock);
> +
> + return ret;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static IIO_CONST_ATTR(in_intensity_calibscale_available, "1 2 64 128");
> +static IIO_CONST_ATTR(in_proximity_calibscale_available, "1 2 4");
> +
> +static struct attribute *rpr0521_attributes[] = {
> + &iio_const_attr_in_intensity_calibscale_available.dev_attr.attr,
> + &iio_const_attr_in_proximity_calibscale_available.dev_attr.attr,
> + NULL
> +};
> +
> +static const struct attribute_group rpr0521_attribute_group = {
> + .attrs = rpr0521_attributes,
> +};
> +
> +static const struct iio_info rpr0521_info = {
> + .driver_module = THIS_MODULE,
> + .read_raw = rpr0521_read_raw,
> + .write_raw = rpr0521_write_raw,
> + .attrs = &rpr0521_attribute_group,
> +};
> +
> +static int rpr0521_init(struct rpr0521_data *data)
> +{
> + int ret;
> + int id;
> +
> + ret = regmap_read(data->regmap, RPR0521_REG_ID, &id);
> + if (ret < 0) {
> + dev_err(&data->client->dev, "Failed to read REG_ID register\n");
> + return ret;
> + }
> +
> + if (id != RPR0521_MANUFACT_ID) {
> + dev_err(&data->client->dev, "Wrong id, got %x expected %x\n",
may be a comma before expected
> + id, RPR0521_MANUFACT_ID);
> + return -ENODEV;
> + }
> +
> + /* set default measurement time - 100 ms for both ALS and PS */
> + ret = regmap_update_bits(data->regmap, RPR0521_REG_MODE_CTRL,
> + RPR0521_MODE_MEAS_TIME_MASK,
> + RPR0521_DEFAULT_MEAS_TIME);
> + if (ret) {
> + pr_err("regmap_update bits returned %d\n", ret);
update_bits
> + return ret;
> + }
> +
> + ret = rpr0521_als_enable(data, RPR0521_MODE_ALS_ENABLE);
> + if (ret < 0)
> + return ret;
> + ret = rpr0521_pxs_enable(data, RPR0521_MODE_PXS_ENABLE);
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int rpr0521_poweroff(struct rpr0521_data *data)
> +{
> + int ret;
> +
> + ret = regmap_update_bits(data->regmap, RPR0521_REG_MODE_CTRL,
> + RPR0521_MODE_ALS_MASK |
> + RPR0521_MODE_PXS_MASK,
> + 0x00);
> + if (ret < 0)
> + return ret;
> +
> + data->als_dev_en = false;
> + data->pxs_dev_en = false;
> +
> + return 0;
> +}
> +
> +static bool rpr0521_is_volatile_reg(struct device *dev, unsigned int reg)
> +{
> + switch (reg) {
> + case RPR0521_REG_MODE_CTRL:
> + case RPR0521_REG_ALS_CTRL:
> + case RPR0521_REG_PXS_CTRL:
> + return false;
> + default:
> + return true;
> + }
> +}
> +
> +static const struct regmap_config rpr0521_regmap_config = {
> + .name = RPR0521_REGMAP_NAME,
> +
> + .reg_bits = 8,
> + .val_bits = 8,
> +
> + .max_register = RPR0521_REG_ID,
> + .cache_type = REGCACHE_RBTREE,
> + .volatile_reg = rpr0521_is_volatile_reg,
> +};
> +
> +static int rpr0521_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct rpr0521_data *data;
> + struct iio_dev *indio_dev;
> + struct regmap *regmap;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + regmap = devm_regmap_init_i2c(client, &rpr0521_regmap_config);
> + if (IS_ERR(regmap)) {
> + dev_err(&client->dev, "Regmap init failed!\n");
regmap_init
> + return PTR_ERR(regmap);
> + }
> +
> + data = iio_priv(indio_dev);
> + i2c_set_clientdata(client, indio_dev);
> + data->client = client;
> + data->regmap = regmap;
> +
> + mutex_init(&data->lock);
> +
> + indio_dev->dev.parent = &client->dev;
> + indio_dev->info = &rpr0521_info;
> + indio_dev->name = RPR0521_DRV_NAME;
> + indio_dev->channels = rpr0521_channels;
> + indio_dev->num_channels = ARRAY_SIZE(rpr0521_channels);
> + indio_dev->modes = INDIO_DIRECT_MODE;
> +
> + ret = rpr0521_init(data);
> + if (ret < 0) {
> + dev_err(&client->dev, "rpr0521 chip init failed\n");
> + return ret;
> + }
> + ret = iio_device_register(indio_dev);
> + if (ret < 0)
> + return ret;
> +
> + ret = pm_runtime_set_active(&client->dev);
> + if (ret < 0)
> + goto err_iio_unregister;
> +
> + pm_runtime_enable(&client->dev);
> + pm_runtime_set_autosuspend_delay(&client->dev, RPR0521_SLEEP_DELAY_MS);
> + pm_runtime_use_autosuspend(&client->dev);
> +
> + return 0;
maybe some whitespace here
> +err_iio_unregister:
> + iio_device_unregister(indio_dev);
> + return ret;
> +}
> +
> +static int rpr0521_remove(struct i2c_client *client)
> +{
> + struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +
> + pm_runtime_disable(&client->dev);
> + pm_runtime_set_suspended(&client->dev);
> + pm_runtime_put_noidle(&client->dev);
> +
> + iio_device_unregister(indio_dev);
> + rpr0521_poweroff(iio_priv(indio_dev));
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int rpr0521_runtime_suspend(struct device *dev)
> +{
> + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> + struct rpr0521_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + /* disable channels and sets {als,pxs}_dev_en to false */
> + mutex_lock(&data->lock);
> + ret = rpr0521_poweroff(data);
> + mutex_unlock(&data->lock);
> +
> + return ret;
> +}
> +
> +static int rpr0521_runtime_resume(struct device *dev)
> +{
> + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> + struct rpr0521_data *data = iio_priv(indio_dev);
> + int ret;
> +
> + if (data->als_ps_need_en) {
> + ret = rpr0521_als_enable(data, RPR0521_MODE_ALS_ENABLE);
> + if (ret < 0)
> + return ret;
> + data->als_ps_need_en = false;
> + }
> +
> + if (data->pxs_ps_need_en) {
> + ret = rpr0521_pxs_enable(data, RPR0521_MODE_PXS_ENABLE);
> + if (ret < 0)
> + return ret;
> + data->pxs_ps_need_en = false;
> + }
> +
> + return 0;
> +}
> +#endif
> +
> +static const struct dev_pm_ops rpr0521_pm_ops = {
> + SET_RUNTIME_PM_OPS(rpr0521_runtime_suspend,
> + rpr0521_runtime_resume, NULL)
> +};
> +
> +static const struct acpi_device_id rpr0521_acpi_match[] = {
> + {"RPR0521", 0},
> + { }
> +};
> +MODULE_DEVICE_TABLE(acpi, rpr0521_acpi_match);
> +
> +static const struct i2c_device_id rpr0521_id[] = {
> + {"rpr0521", 0},
> + { }
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, rpr0521_id);
> +
> +static struct i2c_driver rpr0521_driver = {
> + .driver = {
> + .name = RPR0521_DRV_NAME,
> + .pm = &rpr0521_pm_ops,
> + .acpi_match_table = ACPI_PTR(rpr0521_acpi_match),
> + },
> + .probe = rpr0521_probe,
> + .remove = rpr0521_remove,
> + .id_table = rpr0521_id,
> +};
> +
> +module_i2c_driver(rpr0521_driver);
> +
> +MODULE_AUTHOR("Daniel Baluta <daniel.baluta@intel.com>");
> +MODULE_DESCRIPTION("RPR0521 ROHM Ambient Light and Proximity Sensor driver");
> +MODULE_LICENSE("GPL v2");
>
--
Peter Meerwald
+43-664-2444418 (mobile)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] iio: light: Add support for ROHM RPR0521 sensor
2015-05-28 11:35 ` Peter Meerwald
@ 2015-05-28 12:49 ` Daniel Baluta
2015-05-28 13:34 ` Peter Meerwald
0 siblings, 1 reply; 9+ messages in thread
From: Daniel Baluta @ 2015-05-28 12:49 UTC (permalink / raw)
To: Peter Meerwald
Cc: Daniel Baluta, Jonathan Cameron, Hartmut Knaack,
Lars-Peter Clausen, Linux Kernel Mailing List, linux-iio
On Thu, May 28, 2015 at 2:35 PM, Peter Meerwald <pmeerw@pmeerw.net> wrote:
> On Thu, 28 May 2015, Daniel Baluta wrote:
>
>> This patch adds support for ROHM RPR0521 ambient light and proximity
>> sensor. It offers raw readings for intensity and proximity.
>
> comments below
>
>> Signed-off-by: Daniel Baluta <daniel.baluta@intel.com>
>> ---
>> Changes since v1: (after Jonathan's review)
>> * add static const to some structures
>> * make some local functions static
>> * added clarification for set_power_state function.
>>
>> drivers/iio/light/Kconfig | 11 +
>> drivers/iio/light/Makefile | 1 +
>> drivers/iio/light/rpr0521.c | 596 ++++++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 608 insertions(+)
>> create mode 100644 drivers/iio/light/rpr0521.c
>>
>> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
>> index e6198b7..cbc4677 100644
>> --- a/drivers/iio/light/Kconfig
>> +++ b/drivers/iio/light/Kconfig
>> @@ -168,6 +168,17 @@ config JSA1212
>> To compile this driver as a module, choose M here:
>> the module will be called jsa1212.
>>
>> +config RPR0521
>> + tristate "ROHM RPR0521 ALS and proximity sensor driver"
>> + depends on I2C
>> + select REGMAP_I2C
>> + help
>> + Say Y here if you want to build support for ROHM's RPR0521
>> + ambient light and proximity sensor device.
>> +
>> + To compile this driver as a module, choose M here:
>> + the module will be called rpr0521.
>> +
>> config SENSORS_LM3533
>> tristate "LM3533 ambient light sensor"
>> depends on MFD_LM3533
>> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
>> index e2d50fd..66fc2fd 100644
>> --- a/drivers/iio/light/Makefile
>> +++ b/drivers/iio/light/Makefile
>> @@ -17,6 +17,7 @@ obj-$(CONFIG_HID_SENSOR_ALS) += hid-sensor-als.o
>> obj-$(CONFIG_HID_SENSOR_PROX) += hid-sensor-prox.o
>> obj-$(CONFIG_ISL29125) += isl29125.o
>> obj-$(CONFIG_JSA1212) += jsa1212.o
>> +obj-$(CONFIG_RPR0521) += rpr0521.o
>
> alphabetic order
Ok, it seems that I've looked only at CONFIG_ name, but not at the
actual object name.
>
>> obj-$(CONFIG_SENSORS_LM3533) += lm3533-als.o
>> obj-$(CONFIG_LTR501) += ltr501.o
>> obj-$(CONFIG_SENSORS_TSL2563) += tsl2563.o
>> diff --git a/drivers/iio/light/rpr0521.c b/drivers/iio/light/rpr0521.c
>> new file mode 100644
>> index 0000000..45d4df5
>> --- /dev/null
>> +++ b/drivers/iio/light/rpr0521.c
>> @@ -0,0 +1,596 @@
>> +/*
>> + * RPR-0521 ROHM Ambient Light and Proximity Sensor
>> + *
>> + * Copyright (c) 2015, Intel Corporation.
>> + *
>> + * This file is subject to the terms and conditions of version 2 of
>> + * the GNU General Public License. See the file COPYING in the main
>> + * directory of this archive for more details.
>> + *
>> + * IIO driver for RPR-0521RS (7-bit I2C slave address 0x38).
>> + *
>> + * TODO: illuminance channel, PM support, buffer
>
> PM seems to be supported?
runtime PM is supported, but not system PM.
>
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/init.h>
>> +#include <linux/i2c.h>
>> +#include <linux/regmap.h>
>> +#include <linux/delay.h>
>> +#include <linux/acpi.h>
>> +
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/sysfs.h>
>> +#include <linux/pm_runtime.h>
>> +
>> +#define RPR0521_REG_SYSTEM_CTRL 0x40
>> +#define RPR0521_REG_MODE_CTRL 0x41
>> +#define RPR0521_REG_ALS_CTRL 0x42
>> +#define RPR0521_REG_PXS_CTRL 0x43
>> +#define RPR0521_REG_PXS_DATA 0x44 /* 16-bit, little endian */
>> +#define RPR0521_REG_ALS_DATA0 0x46 /* 16-bit, little endian */
>> +#define RPR0521_REG_ALS_DATA1 0x48 /* 16-bit, little endian */
>> +#define RPR0521_REG_ID 0x92
>> +
>> +#define RPR0521_MODE_ALS_MASK BIT(7)
>> +#define RPR0521_MODE_PXS_MASK BIT(6)
>> +#define RPR0521_MODE_MEAS_TIME_MASK GENMASK(3, 0)
>> +#define RPR0521_ALS_DATA0_GAIN_MASK (BIT(4) | BIT(5))
>> +#define RPR0521_ALS_DATA0_GAIN_SHIFT 4
>> +#define RPR0521_ALS_DATA1_GAIN_MASK (BIT(2) | BIT(3))
>> +#define RPR0521_ALS_DATA1_GAIN_SHIFT 2
>> +#define RPR0521_PXS_GAIN_MASK (BIT(4) | BIT(5))
>> +#define RPR0521_PXS_GAIN_SHIFT 4
>> +
>> +#define RPR0521_MODE_ALS_ENABLE BIT(7)
>> +#define RPR0521_MODE_ALS_DISABLE 0x00
>> +#define RPR0521_MODE_PXS_ENABLE BIT(6)
>> +#define RPR0521_MODE_PXS_DISABLE 0x00
>
> _DISABLE is not used
I see. Better I will add RPR0521_MODE_ALS_DISABLE | RPR0521_MODE_PXS_DISABLE
in rpr0521_poweroff rather than 0x00.
>
>> +
>> +#define RPR0521_PXS_MAX_VAL (BIT(13) - 1)
>> +#define RPR0521_MANUFACT_ID 0xE0
>> +#define RPR0521_DEFAULT_MEAS_TIME 0x06 /* ALS - 100ms, PXS - 100ms */
>> +
>> +#define RPR0521_DRV_NAME "RPR0521"
>> +#define RPR0521_REGMAP_NAME "rpr0521_regmap"
>> +
>> +#define RPR0521_SLEEP_DELAY_MS 2000
>> +
>> +static const int rpr0521_als_gain[4] = {1, 2, 64, 128};
>> +static const int rpr0521_pxs_gain[3] = {1, 2, 4};
>> +
>> +enum rpr0521_channel {
>> + RPR0521_CHAN_ALS_DATA0,
>> + RPR0521_CHAN_ALS_DATA1,
>> + RPR0521_CHAN_PXS,
>> +};
>> +
>> +static const int rpr0521_data_reg[] = {
>> + [RPR0521_CHAN_ALS_DATA0] = RPR0521_REG_ALS_DATA0,
>> + [RPR0521_CHAN_ALS_DATA1] = RPR0521_REG_ALS_DATA1,
>> + [RPR0521_CHAN_PXS] = RPR0521_REG_PXS_DATA,
>> +};
>> +
>> +static const struct rpr0521_gain_info {
>> + u8 reg;
>> + u8 mask;
>> + u8 shift;
>> + int *gain;
>
> why not const int *?
Correct. Will fix.
>
>> + int size;
>> +} rpr0521_gain[] = {
>> + [RPR0521_CHAN_ALS_DATA0] = {
>> + .reg = RPR0521_REG_ALS_CTRL,
>> + .mask = RPR0521_ALS_DATA0_GAIN_MASK,
>> + .shift = RPR0521_ALS_DATA0_GAIN_SHIFT,
>> + .gain = (int *)rpr0521_als_gain,
>> + .size = ARRAY_SIZE(rpr0521_als_gain),
>> + },
>> + [RPR0521_CHAN_ALS_DATA1] = {
>> + .reg = RPR0521_REG_ALS_CTRL,
>> + .mask = RPR0521_ALS_DATA1_GAIN_MASK,
>> + .shift = RPR0521_ALS_DATA1_GAIN_SHIFT,
>> + .gain = (int *)rpr0521_als_gain,
>> + .size = ARRAY_SIZE(rpr0521_als_gain),
>> + },
>> + [RPR0521_CHAN_PXS] = {
>> + .reg = RPR0521_REG_PXS_CTRL,
>> + .mask = RPR0521_PXS_GAIN_MASK,
>> + .shift = RPR0521_PXS_GAIN_SHIFT,
>> + .gain = (int *)rpr0521_pxs_gain,
>> + .size = ARRAY_SIZE(rpr0521_pxs_gain),
>> + },
>> +};
>> +
>> +struct rpr0521_data {
>> + struct i2c_client *client;
>> +
>> + /* protect device params updates (e.g state, gain) */
>> + struct mutex lock;
>> +
>> + /* device active status */
>> + bool als_dev_en;
>> + bool pxs_dev_en;
>> +
>> + /* optimize runtime pm ops - enable device only if needed */
>> + bool als_ps_need_en;
>> + bool pxs_ps_need_en;
>> +
>> + struct regmap *regmap;
>> +};
>> +
>> +static const struct iio_chan_spec rpr0521_channels[] = {
>> + {
>> + .type = IIO_INTENSITY,
>> + .modified = 1,
>> + .address = RPR0521_CHAN_ALS_DATA0,
>> + .channel2 = IIO_MOD_LIGHT_BOTH,
>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>> + BIT(IIO_CHAN_INFO_CALIBSCALE),
>
> why CALIBSCALE and not SCALE?
Because this is used to set/get gain, which is used by the hardware
to do proper scaling.
AFAIK this should be calibscale.
>
>> + },
>> + {
>> + .type = IIO_INTENSITY,
>> + .modified = 1,
>> + .address = RPR0521_CHAN_ALS_DATA1,
>> + .channel2 = IIO_MOD_LIGHT_IR,
>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>> + BIT(IIO_CHAN_INFO_CALIBSCALE),
>> + },
>> + {
>> + .type = IIO_PROXIMITY,
>> + .address = RPR0521_CHAN_PXS,
>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>> + BIT(IIO_CHAN_INFO_CALIBSCALE),
>> + }
>> +};
>> +
>> +static int rpr0521_als_enable(struct rpr0521_data *data, u8 status)
>> +{
>> + int ret;
>> +
>> + ret = regmap_update_bits(data->regmap, RPR0521_REG_MODE_CTRL,
>> + RPR0521_MODE_ALS_MASK,
>> + status);
>> + if (ret < 0)
>> + return ret;
>> +
>> + data->als_dev_en = true;
>> +
>> + return 0;
>> +}
>> +
>> +static int rpr0521_pxs_enable(struct rpr0521_data *data, u8 status)
>> +{
>> + int ret;
>> +
>> + ret = regmap_update_bits(data->regmap, RPR0521_REG_MODE_CTRL,
>> + RPR0521_MODE_PXS_MASK,
>> + status);
>
> status would fit on the line above?
Yes it looks so :). The same for als_enable.
>
>> + if (ret < 0)
>> + return ret;
>> +
>> + data->pxs_dev_en = true;
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * rpr0521_set_power_state - handles runtime PM state and sensors enabled status
>> + *
>> + * @data: rpr0521 device private data
>> + * @on: state to be set for devices in @device_mask
>> + * @device_mask: bitmask specifying for which device we need to update @on state
>> + *
>> + * We rely on rpr0521_runtime_resume to enable our @device_mask devices, but
>> + * if (for example) PXS was enabled (pxs_dev_en = true) by a previous call to
>> + * rpr0521_runtime_resume and we want to enable ALS we MUST set ALS enable
>> + * bit of RPR0521_REG_MODE_CTRL here because rpr0521_runtime_resume will not
>> + * be called twice.
>> + */
>> +static int rpr0521_set_power_state(struct rpr0521_data *data, bool on,
>> + u8 device_mask)
>> +{
>> +#ifdef CONFIG_PM
>> + int ret;
>> + u8 update_mask = 0;
>> +
>> + if (device_mask & RPR0521_MODE_ALS_MASK) {
>> + if (on && !data->als_ps_need_en && data->pxs_dev_en)
>> + update_mask |= RPR0521_MODE_ALS_MASK;
>> + else
>> + data->als_ps_need_en = on;
>> + }
>> +
>> + if (device_mask & RPR0521_MODE_PXS_MASK) {
>> + if (on && !data->pxs_ps_need_en && data->als_dev_en)
>> + update_mask |= RPR0521_MODE_PXS_MASK;
>> + else
>> + data->pxs_ps_need_en = on;
>> + }
>> +
>> + if (update_mask) {
>> + ret = regmap_update_bits(data->regmap, RPR0521_REG_MODE_CTRL,
>> + update_mask, update_mask);
>> + if (ret < 0)
>> + return ret;
>> + }
>> +
>> + if (on) {
>> + ret = pm_runtime_get_sync(&data->client->dev);
>> + } else {
>> + pm_runtime_mark_last_busy(&data->client->dev);
>> + ret = pm_runtime_put_autosuspend(&data->client->dev);
>> + }
>> + if (ret < 0) {
>> + dev_err(&data->client->dev,
>> + "Failed: rpr0521_set_power_state for %d, ret %d\n",
>> + on, ret);
>> + if (on)
>> + pm_runtime_put_noidle(&data->client->dev);
>> +
>> + return ret;
>> + }
>> +#endif
>> + return 0;
>> +}
>> +
>> +static int rpr0521_get_gain_index(int *gainv, int size, int gain)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < size; i++)
>> + if (gain == gainv[i])
>> + return i;
>> +
>> + return -EINVAL;
>> +}
>> +
>> +static int rpr0521_get_gain(struct rpr0521_data *data, int chan, int *val)
>> +{
>> + int ret, reg, idx;
>> +
>> + ret = regmap_read(data->regmap, rpr0521_gain[chan].reg, ®);
>> + if (ret < 0)
>> + return ret;
>> +
>> + idx = (rpr0521_gain[chan].mask & reg) >> rpr0521_gain[chan].shift;
>> + *val = rpr0521_gain[chan].gain[idx];
>> +
>> + return 0;
>> +}
>> +
>> +static int rpr0521_set_gain(struct rpr0521_data *data, int chan, int val)
>> +{
>> + int idx;
>> +
>> + idx = rpr0521_get_gain_index(rpr0521_gain[chan].gain,
>> + rpr0521_gain[chan].size, val);
>> + if (idx < 0)
>> + return idx;
>> +
>> + return regmap_update_bits(data->regmap, rpr0521_gain[chan].reg,
>> + rpr0521_gain[chan].mask,
>> + idx << rpr0521_gain[chan].shift);
>> +}
>> +
>> +static int rpr0521_read_raw(struct iio_dev *indio_dev,
>> + struct iio_chan_spec const *chan, int *val,
>> + int *val2, long mask)
>> +{
>> + struct rpr0521_data *data = iio_priv(indio_dev);
>> + int ret;
>> + u8 device_mask;
>> + __le16 raw_data;
>> +
>> + switch (mask) {
>> + case IIO_CHAN_INFO_RAW:
>> + switch (chan->type) {
>
> could put the mask into _data_reg to avoid the switch
Looks good. Then, here we'll only check for a valid chan->type?
if (chan->type != IIO_INTENSITY && chan->type != IIO_PROXIMITY)
return -EINVAL.
Or we can consider that the chan->type is always valid?
>
>> + case IIO_INTENSITY:
>> + device_mask = RPR0521_MODE_ALS_MASK;
>> + break;
>> + case IIO_PROXIMITY:
>> + device_mask = RPR0521_MODE_PXS_MASK;
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> + mutex_lock(&data->lock);
>> + ret = rpr0521_set_power_state(data, true, device_mask);
>> + if (ret < 0) {
>> + mutex_unlock(&data->lock);
>> + return ret;
>> + }
>> +
>> + ret = regmap_bulk_read(data->regmap,
>> + rpr0521_data_reg[chan->address],
>> + &raw_data, 2);
>> + if (ret < 0) {
>> + rpr0521_set_power_state(data, false, device_mask);
>> + mutex_unlock(&data->lock);
>> + return ret;
>> + }
>> +
>> + ret = rpr0521_set_power_state(data, false, device_mask);
>> + mutex_unlock(&data->lock);
>> + if (ret < 0)
>> + return ret;
>> +
>> + *val = le16_to_cpu(raw_data);
>> + /*
>> + * proximity uses 12 bits, with bits 7:4 of PXS MSB DATA
>> + * being always zero. Also, proximity MUST be exposed as
>> + * a distance with lower values for closer objects.
>
> this matter hasn't been settled, all other proximity sensor drivers do it
> the other way around
Which sensors? It means they do not agree with the ABI:
http://lxr.free-electrons.com/source/Documentation/ABI/testing/sysfs-bus-iio#L1131
>
>> + */
>> + if (chan->type == IIO_PROXIMITY)
>> + *val = RPR0521_PXS_MAX_VAL - *val;
>
> really this should be _PROCESSED, not _RAW?
I understand and it makes sense. Anyhow, looking at
drivers/iio/proximity/sx9500.c
it seems to be using _RAW.
> how to handle it for buffered reads?
Not sure I understand this. Care to add more details :)?
I would expect that for buffer mode we create an item with 12 realbits and
16 storage bits, and to copy the data from register to buffer.
>
>> +
>> + return IIO_VAL_INT;
>> + case IIO_CHAN_INFO_CALIBSCALE:
>> + mutex_lock(&data->lock);
>> + ret = rpr0521_get_gain(data, chan->address, val);
>> + mutex_unlock(&data->lock);
>> + if (ret < 0)
>> + return ret;
>> +
>> + return IIO_VAL_INT;
>> + default:
>> + return -EINVAL;
>> + }
>> +}
>> +
>> +static int rpr0521_write_raw(struct iio_dev *indio_dev,
>> + struct iio_chan_spec const *chan, int val,
>> + int val2, long mask)
>> +{
>> + struct rpr0521_data *data = iio_priv(indio_dev);
>> + int ret;
>> +
>> + switch (mask) {
>> + case IIO_CHAN_INFO_CALIBSCALE:
>> + mutex_lock(&data->lock);
>> + ret = rpr0521_set_gain(data, chan->address, val);
>> + mutex_unlock(&data->lock);
>> +
>> + return ret;
>> + default:
>> + return -EINVAL;
>> + }
>> +}
>> +
>> +static IIO_CONST_ATTR(in_intensity_calibscale_available, "1 2 64 128");
>> +static IIO_CONST_ATTR(in_proximity_calibscale_available, "1 2 4");
>> +
>> +static struct attribute *rpr0521_attributes[] = {
>> + &iio_const_attr_in_intensity_calibscale_available.dev_attr.attr,
>> + &iio_const_attr_in_proximity_calibscale_available.dev_attr.attr,
>> + NULL
>> +};
>> +
>> +static const struct attribute_group rpr0521_attribute_group = {
>> + .attrs = rpr0521_attributes,
>> +};
>> +
>> +static const struct iio_info rpr0521_info = {
>> + .driver_module = THIS_MODULE,
>> + .read_raw = rpr0521_read_raw,
>> + .write_raw = rpr0521_write_raw,
>> + .attrs = &rpr0521_attribute_group,
>> +};
>> +
>> +static int rpr0521_init(struct rpr0521_data *data)
>> +{
>> + int ret;
>> + int id;
>> +
>> + ret = regmap_read(data->regmap, RPR0521_REG_ID, &id);
>> + if (ret < 0) {
>> + dev_err(&data->client->dev, "Failed to read REG_ID register\n");
>> + return ret;
>> + }
>> +
>> + if (id != RPR0521_MANUFACT_ID) {
>> + dev_err(&data->client->dev, "Wrong id, got %x expected %x\n",
>
> may be a comma before expected
Agree.
>
>> + id, RPR0521_MANUFACT_ID);
>> + return -ENODEV;
>> + }
>> +
>> + /* set default measurement time - 100 ms for both ALS and PS */
>> + ret = regmap_update_bits(data->regmap, RPR0521_REG_MODE_CTRL,
>> + RPR0521_MODE_MEAS_TIME_MASK,
>> + RPR0521_DEFAULT_MEAS_TIME);
>> + if (ret) {
>> + pr_err("regmap_update bits returned %d\n", ret);
>
> update_bits
Agree.
>
>> + return ret;
>> + }
>> +
>> + ret = rpr0521_als_enable(data, RPR0521_MODE_ALS_ENABLE);
>> + if (ret < 0)
>> + return ret;
>> + ret = rpr0521_pxs_enable(data, RPR0521_MODE_PXS_ENABLE);
>> + if (ret < 0)
>> + return ret;
>> +
>> + return 0;
>> +}
>> +
>> +static int rpr0521_poweroff(struct rpr0521_data *data)
>> +{
>> + int ret;
>> +
>> + ret = regmap_update_bits(data->regmap, RPR0521_REG_MODE_CTRL,
>> + RPR0521_MODE_ALS_MASK |
>> + RPR0521_MODE_PXS_MASK,
>> + 0x00);
>> + if (ret < 0)
>> + return ret;
>> +
>> + data->als_dev_en = false;
>> + data->pxs_dev_en = false;
>> +
>> + return 0;
>> +}
>> +
>> +static bool rpr0521_is_volatile_reg(struct device *dev, unsigned int reg)
>> +{
>> + switch (reg) {
>> + case RPR0521_REG_MODE_CTRL:
>> + case RPR0521_REG_ALS_CTRL:
>> + case RPR0521_REG_PXS_CTRL:
>> + return false;
>> + default:
>> + return true;
>> + }
>> +}
>> +
>> +static const struct regmap_config rpr0521_regmap_config = {
>> + .name = RPR0521_REGMAP_NAME,
>> +
>> + .reg_bits = 8,
>> + .val_bits = 8,
>> +
>> + .max_register = RPR0521_REG_ID,
>> + .cache_type = REGCACHE_RBTREE,
>> + .volatile_reg = rpr0521_is_volatile_reg,
>> +};
>> +
>> +static int rpr0521_probe(struct i2c_client *client,
>> + const struct i2c_device_id *id)
>> +{
>> + struct rpr0521_data *data;
>> + struct iio_dev *indio_dev;
>> + struct regmap *regmap;
>> + int ret;
>> +
>> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
>> + if (!indio_dev)
>> + return -ENOMEM;
>> +
>> + regmap = devm_regmap_init_i2c(client, &rpr0521_regmap_config);
>> + if (IS_ERR(regmap)) {
>> + dev_err(&client->dev, "Regmap init failed!\n");
>
> regmap_init
sure. :)
>
>> + return PTR_ERR(regmap);
>> + }
>> +
>> + data = iio_priv(indio_dev);
>> + i2c_set_clientdata(client, indio_dev);
>> + data->client = client;
>> + data->regmap = regmap;
>> +
>> + mutex_init(&data->lock);
>> +
>> + indio_dev->dev.parent = &client->dev;
>> + indio_dev->info = &rpr0521_info;
>> + indio_dev->name = RPR0521_DRV_NAME;
>> + indio_dev->channels = rpr0521_channels;
>> + indio_dev->num_channels = ARRAY_SIZE(rpr0521_channels);
>> + indio_dev->modes = INDIO_DIRECT_MODE;
>> +
>> + ret = rpr0521_init(data);
>> + if (ret < 0) {
>> + dev_err(&client->dev, "rpr0521 chip init failed\n");
>> + return ret;
>> + }
>> + ret = iio_device_register(indio_dev);
>> + if (ret < 0)
>> + return ret;
>> +
>> + ret = pm_runtime_set_active(&client->dev);
>> + if (ret < 0)
>> + goto err_iio_unregister;
>> +
>> + pm_runtime_enable(&client->dev);
>> + pm_runtime_set_autosuspend_delay(&client->dev, RPR0521_SLEEP_DELAY_MS);
>> + pm_runtime_use_autosuspend(&client->dev);
>> +
>> + return 0;
>
> maybe some whitespace here
>
do you mean remove the new line? :)
>> +err_iio_unregister:
>> + iio_device_unregister(indio_dev);
>> + return ret;
>> +}
>> +
>> +static int rpr0521_remove(struct i2c_client *client)
>> +{
>> + struct iio_dev *indio_dev = i2c_get_clientdata(client);
>> +
>> + pm_runtime_disable(&client->dev);
>> + pm_runtime_set_suspended(&client->dev);
>> + pm_runtime_put_noidle(&client->dev);
>> +
>> + iio_device_unregister(indio_dev);
>> + rpr0521_poweroff(iio_priv(indio_dev));
>> +
>> + return 0;
>> +}
>> +
>> +#ifdef CONFIG_PM
>> +static int rpr0521_runtime_suspend(struct device *dev)
>> +{
>> + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>> + struct rpr0521_data *data = iio_priv(indio_dev);
>> + int ret;
>> +
>> + /* disable channels and sets {als,pxs}_dev_en to false */
>> + mutex_lock(&data->lock);
>> + ret = rpr0521_poweroff(data);
>> + mutex_unlock(&data->lock);
>> +
>> + return ret;
>> +}
>> +
>> +static int rpr0521_runtime_resume(struct device *dev)
>> +{
>> + struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
>> + struct rpr0521_data *data = iio_priv(indio_dev);
>> + int ret;
>> +
>> + if (data->als_ps_need_en) {
>> + ret = rpr0521_als_enable(data, RPR0521_MODE_ALS_ENABLE);
>> + if (ret < 0)
>> + return ret;
>> + data->als_ps_need_en = false;
>> + }
>> +
>> + if (data->pxs_ps_need_en) {
>> + ret = rpr0521_pxs_enable(data, RPR0521_MODE_PXS_ENABLE);
>> + if (ret < 0)
>> + return ret;
>> + data->pxs_ps_need_en = false;
>> + }
>> +
>> + return 0;
>> +}
>> +#endif
>> +
>> +static const struct dev_pm_ops rpr0521_pm_ops = {
>> + SET_RUNTIME_PM_OPS(rpr0521_runtime_suspend,
>> + rpr0521_runtime_resume, NULL)
>> +};
>> +
>> +static const struct acpi_device_id rpr0521_acpi_match[] = {
>> + {"RPR0521", 0},
>> + { }
>> +};
>> +MODULE_DEVICE_TABLE(acpi, rpr0521_acpi_match);
>> +
>> +static const struct i2c_device_id rpr0521_id[] = {
>> + {"rpr0521", 0},
>> + { }
>> +};
>> +
>> +MODULE_DEVICE_TABLE(i2c, rpr0521_id);
>> +
>> +static struct i2c_driver rpr0521_driver = {
>> + .driver = {
>> + .name = RPR0521_DRV_NAME,
>> + .pm = &rpr0521_pm_ops,
>> + .acpi_match_table = ACPI_PTR(rpr0521_acpi_match),
>> + },
>> + .probe = rpr0521_probe,
>> + .remove = rpr0521_remove,
>> + .id_table = rpr0521_id,
>> +};
>> +
>> +module_i2c_driver(rpr0521_driver);
>> +
>> +MODULE_AUTHOR("Daniel Baluta <daniel.baluta@intel.com>");
>> +MODULE_DESCRIPTION("RPR0521 ROHM Ambient Light and Proximity Sensor driver");
>> +MODULE_LICENSE("GPL v2");
Thanks a lot Peter for review. I will leave this patch one more day
here, then send v3.
Daniel.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] iio: light: Add support for ROHM RPR0521 sensor
2015-05-28 12:49 ` Daniel Baluta
@ 2015-05-28 13:34 ` Peter Meerwald
2015-05-28 14:17 ` Daniel Baluta
0 siblings, 1 reply; 9+ messages in thread
From: Peter Meerwald @ 2015-05-28 13:34 UTC (permalink / raw)
To: Daniel Baluta
Cc: Jonathan Cameron, Hartmut Knaack, Lars-Peter Clausen,
Linux Kernel Mailing List, linux-iio
> >> +static const struct rpr0521_gain_info {
> >> + u8 reg;
> >> + u8 mask;
> >> + u8 shift;
> >> + int *gain;
> >
> > why not const int *?
>
> Correct. Will fix.
>
> >
> >> + int size;
> >> +} rpr0521_gain[] = {
> >> + [RPR0521_CHAN_ALS_DATA0] = {
> >> + .reg = RPR0521_REG_ALS_CTRL,
> >> + .mask = RPR0521_ALS_DATA0_GAIN_MASK,
> >> + .shift = RPR0521_ALS_DATA0_GAIN_SHIFT,
> >> + .gain = (int *)rpr0521_als_gain,
const int* would take care of the cast here as well
> >> + .size = ARRAY_SIZE(rpr0521_als_gain),
> >> + },
> >> + [RPR0521_CHAN_ALS_DATA1] = {
> >> + .reg = RPR0521_REG_ALS_CTRL,
> >> + .mask = RPR0521_ALS_DATA1_GAIN_MASK,
> >> + .shift = RPR0521_ALS_DATA1_GAIN_SHIFT,
> >> + .gain = (int *)rpr0521_als_gain,
> >> + .size = ARRAY_SIZE(rpr0521_als_gain),
> >> + },
> >> + [RPR0521_CHAN_PXS] = {
> >> + .reg = RPR0521_REG_PXS_CTRL,
> >> + .mask = RPR0521_PXS_GAIN_MASK,
> >> + .shift = RPR0521_PXS_GAIN_SHIFT,
> >> + .gain = (int *)rpr0521_pxs_gain,
> >> + .size = ARRAY_SIZE(rpr0521_pxs_gain),
> >> + },
> >> +};
> >> +
> >> +struct rpr0521_data {
> >> + struct i2c_client *client;
> >> +
> >> + /* protect device params updates (e.g state, gain) */
> >> + struct mutex lock;
> >> +
> >> + /* device active status */
> >> + bool als_dev_en;
> >> + bool pxs_dev_en;
> >> +
> >> + /* optimize runtime pm ops - enable device only if needed */
> >> + bool als_ps_need_en;
> >> + bool pxs_ps_need_en;
> >> +
> >> + struct regmap *regmap;
> >> +};
> >> +
> >> +static const struct iio_chan_spec rpr0521_channels[] = {
> >> + {
> >> + .type = IIO_INTENSITY,
> >> + .modified = 1,
> >> + .address = RPR0521_CHAN_ALS_DATA0,
> >> + .channel2 = IIO_MOD_LIGHT_BOTH,
> >> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> >> + BIT(IIO_CHAN_INFO_CALIBSCALE),
> >
> > why CALIBSCALE and not SCALE?
>
> Because this is used to set/get gain, which is used by the hardware
> to do proper scaling.
>
> AFAIK this should be calibscale.
in sysfs-bus-iiof on CALIBSCALE: Hardware applied calibration scale factor
(assumed to fix production inaccuracies).
this doesn't seem applicable here, it is a gain factor controlling
measurement resolution
> >
> >> + },
> >> + {
> >> + .type = IIO_INTENSITY,
> >> + .modified = 1,
> >> + .address = RPR0521_CHAN_ALS_DATA1,
> >> + .channel2 = IIO_MOD_LIGHT_IR,
> >> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> >> + BIT(IIO_CHAN_INFO_CALIBSCALE),
> >> + },
> >> + {
> >> + .type = IIO_PROXIMITY,
> >> + .address = RPR0521_CHAN_PXS,
> >> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> >> + BIT(IIO_CHAN_INFO_CALIBSCALE),
> >> + }
> >> +};
> >> +
> >> +static int rpr0521_als_enable(struct rpr0521_data *data, u8 status)
> >> +{
> >> + int ret;
> >> +
> >> + ret = regmap_update_bits(data->regmap, RPR0521_REG_MODE_CTRL,
> >> + RPR0521_MODE_ALS_MASK,
> >> + status);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + data->als_dev_en = true;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int rpr0521_pxs_enable(struct rpr0521_data *data, u8 status)
> >> +{
> >> + int ret;
> >> +
> >> + ret = regmap_update_bits(data->regmap, RPR0521_REG_MODE_CTRL,
> >> + RPR0521_MODE_PXS_MASK,
> >> + status);
> >
> > status would fit on the line above?
>
> Yes it looks so :). The same for als_enable.
>
> >
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + data->pxs_dev_en = true;
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +/**
> >> + * rpr0521_set_power_state - handles runtime PM state and sensors enabled status
> >> + *
> >> + * @data: rpr0521 device private data
> >> + * @on: state to be set for devices in @device_mask
> >> + * @device_mask: bitmask specifying for which device we need to update @on state
> >> + *
> >> + * We rely on rpr0521_runtime_resume to enable our @device_mask devices, but
> >> + * if (for example) PXS was enabled (pxs_dev_en = true) by a previous call to
> >> + * rpr0521_runtime_resume and we want to enable ALS we MUST set ALS enable
> >> + * bit of RPR0521_REG_MODE_CTRL here because rpr0521_runtime_resume will not
> >> + * be called twice.
> >> + */
> >> +static int rpr0521_set_power_state(struct rpr0521_data *data, bool on,
> >> + u8 device_mask)
> >> +{
> >> +#ifdef CONFIG_PM
> >> + int ret;
> >> + u8 update_mask = 0;
> >> +
> >> + if (device_mask & RPR0521_MODE_ALS_MASK) {
> >> + if (on && !data->als_ps_need_en && data->pxs_dev_en)
> >> + update_mask |= RPR0521_MODE_ALS_MASK;
> >> + else
> >> + data->als_ps_need_en = on;
> >> + }
> >> +
> >> + if (device_mask & RPR0521_MODE_PXS_MASK) {
> >> + if (on && !data->pxs_ps_need_en && data->als_dev_en)
> >> + update_mask |= RPR0521_MODE_PXS_MASK;
> >> + else
> >> + data->pxs_ps_need_en = on;
> >> + }
> >> +
> >> + if (update_mask) {
> >> + ret = regmap_update_bits(data->regmap, RPR0521_REG_MODE_CTRL,
> >> + update_mask, update_mask);
> >> + if (ret < 0)
> >> + return ret;
> >> + }
> >> +
> >> + if (on) {
> >> + ret = pm_runtime_get_sync(&data->client->dev);
> >> + } else {
> >> + pm_runtime_mark_last_busy(&data->client->dev);
> >> + ret = pm_runtime_put_autosuspend(&data->client->dev);
> >> + }
> >> + if (ret < 0) {
> >> + dev_err(&data->client->dev,
> >> + "Failed: rpr0521_set_power_state for %d, ret %d\n",
> >> + on, ret);
> >> + if (on)
> >> + pm_runtime_put_noidle(&data->client->dev);
> >> +
> >> + return ret;
> >> + }
> >> +#endif
> >> + return 0;
> >> +}
> >> +
> >> +static int rpr0521_get_gain_index(int *gainv, int size, int gain)
> >> +{
> >> + int i;
> >> +
> >> + for (i = 0; i < size; i++)
> >> + if (gain == gainv[i])
> >> + return i;
> >> +
> >> + return -EINVAL;
> >> +}
> >> +
> >> +static int rpr0521_get_gain(struct rpr0521_data *data, int chan, int *val)
> >> +{
> >> + int ret, reg, idx;
> >> +
> >> + ret = regmap_read(data->regmap, rpr0521_gain[chan].reg, ®);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + idx = (rpr0521_gain[chan].mask & reg) >> rpr0521_gain[chan].shift;
> >> + *val = rpr0521_gain[chan].gain[idx];
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +static int rpr0521_set_gain(struct rpr0521_data *data, int chan, int val)
> >> +{
> >> + int idx;
> >> +
> >> + idx = rpr0521_get_gain_index(rpr0521_gain[chan].gain,
> >> + rpr0521_gain[chan].size, val);
> >> + if (idx < 0)
> >> + return idx;
> >> +
> >> + return regmap_update_bits(data->regmap, rpr0521_gain[chan].reg,
> >> + rpr0521_gain[chan].mask,
> >> + idx << rpr0521_gain[chan].shift);
> >> +}
> >> +
> >> +static int rpr0521_read_raw(struct iio_dev *indio_dev,
> >> + struct iio_chan_spec const *chan, int *val,
> >> + int *val2, long mask)
> >> +{
> >> + struct rpr0521_data *data = iio_priv(indio_dev);
> >> + int ret;
> >> + u8 device_mask;
> >> + __le16 raw_data;
> >> +
> >> + switch (mask) {
> >> + case IIO_CHAN_INFO_RAW:
> >> + switch (chan->type) {
> >
> > could put the mask into _data_reg to avoid the switch
> Looks good. Then, here we'll only check for a valid chan->type?
>
> if (chan->type != IIO_INTENSITY && chan->type != IIO_PROXIMITY)
> return -EINVAL.
>
> Or we can consider that the chan->type is always valid?
I'd think so; you also assume that chan->address is valid
I suggest to use chan->address to point to a table containing the
address and the mask
>
> >
> >> + case IIO_INTENSITY:
> >> + device_mask = RPR0521_MODE_ALS_MASK;
> >> + break;
> >> + case IIO_PROXIMITY:
> >> + device_mask = RPR0521_MODE_PXS_MASK;
> >> + break;
> >> + default:
> >> + return -EINVAL;
> >> + }
> >> + mutex_lock(&data->lock);
> >> + ret = rpr0521_set_power_state(data, true, device_mask);
> >> + if (ret < 0) {
> >> + mutex_unlock(&data->lock);
> >> + return ret;
> >> + }
> >> +
> >> + ret = regmap_bulk_read(data->regmap,
> >> + rpr0521_data_reg[chan->address],
> >> + &raw_data, 2);
> >> + if (ret < 0) {
> >> + rpr0521_set_power_state(data, false, device_mask);
> >> + mutex_unlock(&data->lock);
> >> + return ret;
> >> + }
> >> +
> >> + ret = rpr0521_set_power_state(data, false, device_mask);
> >> + mutex_unlock(&data->lock);
> >> + if (ret < 0)
> >> + return ret;
> >> +
> >> + *val = le16_to_cpu(raw_data);
> >> + /*
> >> + * proximity uses 12 bits, with bits 7:4 of PXS MSB DATA
> >> + * being always zero. Also, proximity MUST be exposed as
> >> + * a distance with lower values for closer objects.
> >
> > this matter hasn't been settled, all other proximity sensor drivers do it
> > the other way around
>
> Which sensors? It means they do not agree with the ABI:
>
> http://lxr.free-electrons.com/source/Documentation/ABI/testing/sysfs-bus-iio#L1131
that 'clarification' was added recently,
614e8842ddf5502f0e781f91695bfbc1e1e1d9b6 (with 3.18)
"Proximity measurement .. by observing reflectivity"
high proximity <-> high reflectivity -- this is the reality of what most
sensors output (including yours)
proximity and distance are opposite concepts;
high proximity <-> low distance, and vice versa
the distance part doesn't make sense in the ABI description
> >
> >> + */
> >> + if (chan->type == IIO_PROXIMITY)
> >> + *val = RPR0521_PXS_MAX_VAL - *val;
> >
> > really this should be _PROCESSED, not _RAW?
>
> I understand and it makes sense. Anyhow, looking at
> drivers/iio/proximity/sx9500.c
> it seems to be using _RAW.
>
> > how to handle it for buffered reads?
>
> Not sure I understand this. Care to add more details :)?
> I would expect that for buffer mode we create an item with 12 realbits and
> 16 storage bits, and to copy the data from register to buffer.
in buffered mode we want to avoid manipulating the data (i.e. MAX_DATA -
measurement_value)
since MAX_DATA is not exposed, user mode cannot do this computation and
_RAW differs from the buffered output
(I assume that we want to have buffered output correspond to _RAW values)
> >> + pm_runtime_set_autosuspend_delay(&client->dev, RPR0521_SLEEP_DELAY_MS);
> >> + pm_runtime_use_autosuspend(&client->dev);
> >> +
> >> + return 0;
> >
> > maybe some whitespace here
> >
> do you mean remove the new line? :)
add a newline if I predict Jonathan's perference correctly :)
--
Peter Meerwald
+43-664-2444418 (mobile)
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] iio: light: Add support for ROHM RPR0521 sensor
2015-05-28 13:34 ` Peter Meerwald
@ 2015-05-28 14:17 ` Daniel Baluta
2015-06-03 8:56 ` Daniel Baluta
0 siblings, 1 reply; 9+ messages in thread
From: Daniel Baluta @ 2015-05-28 14:17 UTC (permalink / raw)
To: Peter Meerwald
Cc: Daniel Baluta, Jonathan Cameron, Hartmut Knaack,
Lars-Peter Clausen, Linux Kernel Mailing List, linux-iio,
Vlad Dogaru
<snip>
>> >> +static const struct iio_chan_spec rpr0521_channels[] = {
>> >> + {
>> >> + .type = IIO_INTENSITY,
>> >> + .modified = 1,
>> >> + .address = RPR0521_CHAN_ALS_DATA0,
>> >> + .channel2 = IIO_MOD_LIGHT_BOTH,
>> >> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>> >> + BIT(IIO_CHAN_INFO_CALIBSCALE),
>> >
>> > why CALIBSCALE and not SCALE?
>>
>> Because this is used to set/get gain, which is used by the hardware
>> to do proper scaling.
>>
>> AFAIK this should be calibscale.
>
> in sysfs-bus-iiof on CALIBSCALE: Hardware applied calibration scale factor
> (assumed to fix production inaccuracies).
>
> this doesn't seem applicable here, it is a gain factor controlling
> measurement resolution
Ok, I see now and it makes sense :).
# echo 1 > in_intensity_ir_calibscale
# cat in_intensity_ir_raw
79
# echo 64 > in_intensity_ir_calibscale
# cat in_intensity_ir_raw
5084
The user should get the same value regardless of the gain :), and in the
above example for x64 gain it should have a 1/64 scale.
<snip>
>> Or we can consider that the chan->type is always valid?
>
> I'd think so; you also assume that chan->address is valid
>
> I suggest to use chan->address to point to a table containing the
> address and the mask
<snip>
>> Which sensors? It means they do not agree with the ABI:
>>
>> http://lxr.free-electrons.com/source/Documentation/ABI/testing/sysfs-bus-iio#L1131
>
> that 'clarification' was added recently,
> 614e8842ddf5502f0e781f91695bfbc1e1e1d9b6 (with 3.18)
> "Proximity measurement .. by observing reflectivity"
>
> high proximity <-> high reflectivity -- this is the reality of what most
> sensors output (including yours)
>
> proximity and distance are opposite concepts;
> high proximity <-> low distance, and vice versa
>
> the distance part doesn't make sense in the ABI description
At least sx9500 uses this convention and userspace applications rely on this.
>
>> >
>> >> + */
>> >> + if (chan->type == IIO_PROXIMITY)
>> >> + *val = RPR0521_PXS_MAX_VAL - *val;
>> >
>> > really this should be _PROCESSED, not _RAW?
>>
>> I understand and it makes sense. Anyhow, looking at
>> drivers/iio/proximity/sx9500.c
>> it seems to be using _RAW.
>>
>> > how to handle it for buffered reads?
>>
>> Not sure I understand this. Care to add more details :)?
>
>> I would expect that for buffer mode we create an item with 12 realbits and
>> 16 storage bits, and to copy the data from register to buffer.
>
> in buffered mode we want to avoid manipulating the data (i.e. MAX_DATA -
> measurement_value)
>
> since MAX_DATA is not exposed, user mode cannot do this computation and
> _RAW differs from the buffered output
>
> (I assume that we want to have buffered output correspond to _RAW values)
>
>> >> + pm_runtime_set_autosuspend_delay(&client->dev, RPR0521_SLEEP_DELAY_MS);
>> >> + pm_runtime_use_autosuspend(&client->dev);
>> >> +
>> >> + return 0;
>> >
>> > maybe some whitespace here
>> >
>> do you mean remove the new line? :)
>
> add a newline if I predict Jonathan's perference correctly :)
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] iio: light: Add support for ROHM RPR0521 sensor
2015-05-28 14:17 ` Daniel Baluta
@ 2015-06-03 8:56 ` Daniel Baluta
2015-06-06 21:08 ` Jonathan Cameron
0 siblings, 1 reply; 9+ messages in thread
From: Daniel Baluta @ 2015-06-03 8:56 UTC (permalink / raw)
To: Daniel Baluta
Cc: Peter Meerwald, Jonathan Cameron, Hartmut Knaack,
Lars-Peter Clausen, Linux Kernel Mailing List, linux-iio,
Vlad Dogaru, Tiberiu Breana
On Thu, May 28, 2015 at 5:17 PM, Daniel Baluta <daniel.baluta@intel.com> wrote:
> <snip>
>
>>> >> +static const struct iio_chan_spec rpr0521_channels[] = {
>>> >> + {
>>> >> + .type = IIO_INTENSITY,
>>> >> + .modified = 1,
>>> >> + .address = RPR0521_CHAN_ALS_DATA0,
>>> >> + .channel2 = IIO_MOD_LIGHT_BOTH,
>>> >> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>>> >> + BIT(IIO_CHAN_INFO_CALIBSCALE),
>>> >
>>> > why CALIBSCALE and not SCALE?
>>>
>>> Because this is used to set/get gain, which is used by the hardware
>>> to do proper scaling.
>>>
>>> AFAIK this should be calibscale.
>>
>> in sysfs-bus-iiof on CALIBSCALE: Hardware applied calibration scale factor
>> (assumed to fix production inaccuracies).
>>
>> this doesn't seem applicable here, it is a gain factor controlling
>> measurement resolution
>
> Ok, I see now and it makes sense :).
>
> # echo 1 > in_intensity_ir_calibscale
> # cat in_intensity_ir_raw
> 79
> # echo 64 > in_intensity_ir_calibscale
> # cat in_intensity_ir_raw
> 5084
>
> The user should get the same value regardless of the gain :), and in the
> above example for x64 gain it should have a 1/64 scale.
>
>
> <snip>
>
>>> Or we can consider that the chan->type is always valid?
>>
>> I'd think so; you also assume that chan->address is valid
>>
>> I suggest to use chan->address to point to a table containing the
>> address and the mask
>
> <snip>
>
>>> Which sensors? It means they do not agree with the ABI:
>>>
>>> http://lxr.free-electrons.com/source/Documentation/ABI/testing/sysfs-bus-iio#L1131
>>
>> that 'clarification' was added recently,
>> 614e8842ddf5502f0e781f91695bfbc1e1e1d9b6 (with 3.18)
>> "Proximity measurement .. by observing reflectivity"
>>
>> high proximity <-> high reflectivity -- this is the reality of what most
>> sensors output (including yours)
>>
>> proximity and distance are opposite concepts;
>> high proximity <-> low distance, and vice versa
>>
>> the distance part doesn't make sense in the ABI description
>
> At least sx9500 uses this convention and userspace applications rely on this.
OK, so wee need to agree on this part and to add a proper descriptor to the ABI.
Jonathan, what do you say?
Daniel.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] iio: light: Add support for ROHM RPR0521 sensor
2015-06-03 8:56 ` Daniel Baluta
@ 2015-06-06 21:08 ` Jonathan Cameron
2015-06-11 9:19 ` Daniel Baluta
0 siblings, 1 reply; 9+ messages in thread
From: Jonathan Cameron @ 2015-06-06 21:08 UTC (permalink / raw)
To: Daniel Baluta
Cc: Peter Meerwald, Hartmut Knaack, Lars-Peter Clausen,
Linux Kernel Mailing List, linux-iio, Vlad Dogaru,
Tiberiu Breana
On 06/03/2015 09:56 AM, Daniel Baluta wrote:
> On Thu, May 28, 2015 at 5:17 PM, Daniel Baluta <daniel.baluta@intel.com> wrote:
>> <snip>
>>
>>>>>> +static const struct iio_chan_spec rpr0521_channels[] = {
>>>>>> + {
>>>>>> + .type = IIO_INTENSITY,
>>>>>> + .modified = 1,
>>>>>> + .address = RPR0521_CHAN_ALS_DATA0,
>>>>>> + .channel2 = IIO_MOD_LIGHT_BOTH,
>>>>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>>>>>> + BIT(IIO_CHAN_INFO_CALIBSCALE),
>>>>>
>>>>> why CALIBSCALE and not SCALE?
>>>>
>>>> Because this is used to set/get gain, which is used by the hardware
>>>> to do proper scaling.
>>>>
>>>> AFAIK this should be calibscale.
>>>
>>> in sysfs-bus-iiof on CALIBSCALE: Hardware applied calibration scale factor
>>> (assumed to fix production inaccuracies).
>>>
>>> this doesn't seem applicable here, it is a gain factor controlling
>>> measurement resolution
>>
>> Ok, I see now and it makes sense :).
>>
>> # echo 1 > in_intensity_ir_calibscale
>> # cat in_intensity_ir_raw
>> 79
>> # echo 64 > in_intensity_ir_calibscale
>> # cat in_intensity_ir_raw
>> 5084
>>
>> The user should get the same value regardless of the gain :), and in the
>> above example for x64 gain it should have a 1/64 scale.
>>
>>
>> <snip>
>>
>>>> Or we can consider that the chan->type is always valid?
>>>
>>> I'd think so; you also assume that chan->address is valid
>>>
>>> I suggest to use chan->address to point to a table containing the
>>> address and the mask
>>
>> <snip>
>>
>>>> Which sensors? It means they do not agree with the ABI:
>>>>
>>>> http://lxr.free-electrons.com/source/Documentation/ABI/testing/sysfs-bus-iio#L1131
>>>
>>> that 'clarification' was added recently,
>>> 614e8842ddf5502f0e781f91695bfbc1e1e1d9b6 (with 3.18)
>>> "Proximity measurement .. by observing reflectivity"
>>>
>>> high proximity <-> high reflectivity -- this is the reality of what most
>>> sensors output (including yours)
>>>
>>> proximity and distance are opposite concepts;
>>> high proximity <-> low distance, and vice versa
>>>
>>> the distance part doesn't make sense in the ABI description
>>
>> At least sx9500 uses this convention and userspace applications rely on this.
>
> OK, so wee need to agree on this part and to add a proper descriptor to the ABI.
>
> Jonathan, what do you say?
>
I agree that we need to agree one way or the other. Proximity being
higher as you get closer seems slightly more logical to me
(I wish now that I'd argued in favour of just doing distance, but such
is hindsight). Still I'm happy with whatever consensus forms.
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] iio: light: Add support for ROHM RPR0521 sensor
2015-06-06 21:08 ` Jonathan Cameron
@ 2015-06-11 9:19 ` Daniel Baluta
2015-06-11 14:18 ` jic23
0 siblings, 1 reply; 9+ messages in thread
From: Daniel Baluta @ 2015-06-11 9:19 UTC (permalink / raw)
To: Jonathan Cameron, mranostay
Cc: Daniel Baluta, Peter Meerwald, Hartmut Knaack,
Lars-Peter Clausen, Linux Kernel Mailing List, linux-iio,
Vlad Dogaru, Tiberiu Breana
On Sun, Jun 7, 2015 at 12:08 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>
>
> On 06/03/2015 09:56 AM, Daniel Baluta wrote:
>>
>> On Thu, May 28, 2015 at 5:17 PM, Daniel Baluta <daniel.baluta@intel.com>
>> wrote:
>>>
>>> <snip>
>>>
>>>>>>> +static const struct iio_chan_spec rpr0521_channels[] = {
>>>>>>> + {
>>>>>>> + .type = IIO_INTENSITY,
>>>>>>> + .modified = 1,
>>>>>>> + .address = RPR0521_CHAN_ALS_DATA0,
>>>>>>> + .channel2 = IIO_MOD_LIGHT_BOTH,
>>>>>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>>>>>>> + BIT(IIO_CHAN_INFO_CALIBSCALE),
>>>>>>
>>>>>>
>>>>>> why CALIBSCALE and not SCALE?
>>>>>
>>>>>
>>>>> Because this is used to set/get gain, which is used by the hardware
>>>>> to do proper scaling.
>>>>>
>>>>> AFAIK this should be calibscale.
>>>>
>>>>
>>>> in sysfs-bus-iiof on CALIBSCALE: Hardware applied calibration scale
>>>> factor
>>>> (assumed to fix production inaccuracies).
>>>>
>>>> this doesn't seem applicable here, it is a gain factor controlling
>>>> measurement resolution
>>>
>>>
>>> Ok, I see now and it makes sense :).
>>>
>>> # echo 1 > in_intensity_ir_calibscale
>>> # cat in_intensity_ir_raw
>>> 79
>>> # echo 64 > in_intensity_ir_calibscale
>>> # cat in_intensity_ir_raw
>>> 5084
>>>
>>> The user should get the same value regardless of the gain :), and in the
>>> above example for x64 gain it should have a 1/64 scale.
>>>
>>>
>>> <snip>
>>>
>>>>> Or we can consider that the chan->type is always valid?
>>>>
>>>>
>>>> I'd think so; you also assume that chan->address is valid
>>>>
>>>> I suggest to use chan->address to point to a table containing the
>>>> address and the mask
>>>
>>>
>>> <snip>
>>>
>>>>> Which sensors? It means they do not agree with the ABI:
>>>>>
>>>>>
>>>>> http://lxr.free-electrons.com/source/Documentation/ABI/testing/sysfs-bus-iio#L1131
>>>>
>>>>
>>>> that 'clarification' was added recently,
>>>> 614e8842ddf5502f0e781f91695bfbc1e1e1d9b6 (with 3.18)
>>>> "Proximity measurement .. by observing reflectivity"
>>>>
>>>> high proximity <-> high reflectivity -- this is the reality of what most
>>>> sensors output (including yours)
>>>>
>>>> proximity and distance are opposite concepts;
>>>> high proximity <-> low distance, and vice versa
>>>>
>>>> the distance part doesn't make sense in the ABI description
>>>
>>>
>>> At least sx9500 uses this convention and userspace applications rely on
>>> this.
>>
>>
>> OK, so wee need to agree on this part and to add a proper descriptor to
>> the ABI.
>>
>> Jonathan, what do you say?
>>
> I agree that we need to agree one way or the other. Proximity being higher
> as you get closer seems slightly more logical to me
> (I wish now that I'd argued in favour of just doing distance, but such
> is hindsight). Still I'm happy with whatever consensus forms.
+ Matt.
Ok, now I see where the ambiguity comes from. The ABI for proximity also
covers AS3935 Franklin Lightning Sensor IC, where sensor's output provides
an estimation on the distance to the head of a storm :).
But, I don't think this is the type of proximity most of people think of :).
I am not sure how to modify the ABI without breaking the AS3935, but I will
think of a solution and send a RFC as soon as possible.
thanks,
Daniel.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] iio: light: Add support for ROHM RPR0521 sensor
2015-06-11 9:19 ` Daniel Baluta
@ 2015-06-11 14:18 ` jic23
0 siblings, 0 replies; 9+ messages in thread
From: jic23 @ 2015-06-11 14:18 UTC (permalink / raw)
To: Daniel Baluta
Cc: Jonathan Cameron, mranostay, Peter Meerwald, Hartmut Knaack,
Lars-Peter Clausen, Linux Kernel Mailing List, linux-iio,
Vlad Dogaru, Tiberiu Breana
Daniel Baluta writes:
> On Sun, Jun 7, 2015 at 12:08 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>>
>>
>> On 06/03/2015 09:56 AM, Daniel Baluta wrote:
>>>
>>> On Thu, May 28, 2015 at 5:17 PM, Daniel Baluta <daniel.baluta@intel.com>
>>> wrote:
>>>>
>>>> <snip>
>>>>
>>>>>>>> +static const struct iio_chan_spec rpr0521_channels[] = {
>>>>>>>> + {
>>>>>>>> + .type = IIO_INTENSITY,
>>>>>>>> + .modified = 1,
>>>>>>>> + .address = RPR0521_CHAN_ALS_DATA0,
>>>>>>>> + .channel2 = IIO_MOD_LIGHT_BOTH,
>>>>>>>> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>>>>>>>> + BIT(IIO_CHAN_INFO_CALIBSCALE),
>>>>>>>
>>>>>>>
>>>>>>> why CALIBSCALE and not SCALE?
>>>>>>
>>>>>>
>>>>>> Because this is used to set/get gain, which is used by the hardware
>>>>>> to do proper scaling.
>>>>>>
>>>>>> AFAIK this should be calibscale.
>>>>>
>>>>>
>>>>> in sysfs-bus-iiof on CALIBSCALE: Hardware applied calibration scale
>>>>> factor
>>>>> (assumed to fix production inaccuracies).
>>>>>
>>>>> this doesn't seem applicable here, it is a gain factor controlling
>>>>> measurement resolution
>>>>
>>>>
>>>> Ok, I see now and it makes sense :).
>>>>
>>>> # echo 1 > in_intensity_ir_calibscale
>>>> # cat in_intensity_ir_raw
>>>> 79
>>>> # echo 64 > in_intensity_ir_calibscale
>>>> # cat in_intensity_ir_raw
>>>> 5084
>>>>
>>>> The user should get the same value regardless of the gain :), and in the
>>>> above example for x64 gain it should have a 1/64 scale.
>>>>
>>>>
>>>> <snip>
>>>>
>>>>>> Or we can consider that the chan->type is always valid?
>>>>>
>>>>>
>>>>> I'd think so; you also assume that chan->address is valid
>>>>>
>>>>> I suggest to use chan->address to point to a table containing the
>>>>> address and the mask
>>>>
>>>>
>>>> <snip>
>>>>
>>>>>> Which sensors? It means they do not agree with the ABI:
>>>>>>
>>>>>>
>>>>>> http://lxr.free-electrons.com/source/Documentation/ABI/testing/sysfs-bus-iio#L1131
>>>>>
>>>>>
>>>>> that 'clarification' was added recently,
>>>>> 614e8842ddf5502f0e781f91695bfbc1e1e1d9b6 (with 3.18)
>>>>> "Proximity measurement .. by observing reflectivity"
>>>>>
>>>>> high proximity <-> high reflectivity -- this is the reality of what most
>>>>> sensors output (including yours)
>>>>>
>>>>> proximity and distance are opposite concepts;
>>>>> high proximity <-> low distance, and vice versa
>>>>>
>>>>> the distance part doesn't make sense in the ABI description
>>>>
>>>>
>>>> At least sx9500 uses this convention and userspace applications rely on
>>>> this.
>>>
>>>
>>> OK, so wee need to agree on this part and to add a proper descriptor to
>>> the ABI.
>>>
>>> Jonathan, what do you say?
>>>
>> I agree that we need to agree one way or the other. Proximity being higher
>> as you get closer seems slightly more logical to me
>> (I wish now that I'd argued in favour of just doing distance, but such
>> is hindsight). Still I'm happy with whatever consensus forms.
>
> + Matt.
>
> Ok, now I see where the ambiguity comes from. The ABI for proximity also
> covers AS3935 Franklin Lightning Sensor IC, where sensor's output provides
> an estimation on the distance to the head of a storm :).
>
> But, I don't think this is the type of proximity most of people think of :).
>
> I am not sure how to modify the ABI without breaking the AS3935, but I will
> think of a solution and send a RFC as soon as possible.
>
> thanks,
> Daniel.
Hmm. We could define new ABI and deprecate the old one I suppose. Or
take the view that ABI breakage fixes are aloud as long as we have both
options in the tree currently and hence are defining one of them to be
wrong.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2015-06-11 14:18 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-05-28 10:52 [PATCH v2] iio: light: Add support for ROHM RPR0521 sensor Daniel Baluta
2015-05-28 11:35 ` Peter Meerwald
2015-05-28 12:49 ` Daniel Baluta
2015-05-28 13:34 ` Peter Meerwald
2015-05-28 14:17 ` Daniel Baluta
2015-06-03 8:56 ` Daniel Baluta
2015-06-06 21:08 ` Jonathan Cameron
2015-06-11 9:19 ` Daniel Baluta
2015-06-11 14:18 ` jic23
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).