[PATCH v2 0/5] Support for max44000 Ambient and Infrared Proximity Sensor

[PATCH v2 0/5] Support for max44000 Ambient and Infrared Proximity Sensor

[Crestez Dan Leonard]

Changes since v1:
* Remove max44000_data.client
* Maintain alphabetical order in kconfig/makefile
* Avoid pointless inlines
* Remove explicit CONFIG_OF because no special handling is done
* Set storagebits = 16 for proximity channel and avoid VLAs
* Expand MAX44000_REGMASK for readability/reviewability
* Clear irq bits at the end of the probe function
* Only write important bits on probe rather than everything

I'm not sure about only partially resetting the device on probe.

Crestez Dan Leonard (5):
  max44000: Initial support
  max44000: Initial support for proximity reading
  max44000: Support controlling LED current output
  max44000: Expose ambient sensor scaling
  max44000: Initial triggered buffer support

 drivers/iio/light/Kconfig    |  11 +
 drivers/iio/light/Makefile   |   1 +
 drivers/iio/light/max44000.c | 649 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 661 insertions(+)
 create mode 100644 drivers/iio/light/max44000.c

-- 
2.5.5

[PATCH v2 1/5] max44000: Initial support

[Crestez Dan Leonard]

This just adds support for reporting illuminance with default settings.

Important default registers are written on probe because the device
otherwise lacks a reset function.

Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
---
 drivers/iio/light/Kconfig    |  11 ++
 drivers/iio/light/Makefile   |   1 +
 drivers/iio/light/max44000.c | 336 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 348 insertions(+)
 create mode 100644 drivers/iio/light/max44000.c

diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index cfd3df8..b4ab65b 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -223,6 +223,17 @@ config LTR501
 	 This driver can also be built as a module.  If so, the module
          will be called ltr501.
 
+config MAX44000
+	tristate "MAX44000 Ambient and Infrared Proximity Sensor"
+	depends on I2C
+	select REGMAP_I2C
+	help
+	 Say Y here if you want to build support for Maxim Integrated's
+	 MAX44000 ambient and infrared proximity sensor device.
+
+	 To compile this driver as a module, choose M here:
+	 the module will be called max44000.
+
 config OPT3001
 	tristate "Texas Instruments OPT3001 Light Sensor"
 	depends on I2C
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index b2c3105..c77f27f 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -20,6 +20,7 @@ obj-$(CONFIG_ISL29125)		+= isl29125.o
 obj-$(CONFIG_JSA1212)		+= jsa1212.o
 obj-$(CONFIG_SENSORS_LM3533)	+= lm3533-als.o
 obj-$(CONFIG_LTR501)		+= ltr501.o
+obj-$(CONFIG_MAX44000)		+= max44000.o
 obj-$(CONFIG_OPT3001)		+= opt3001.o
 obj-$(CONFIG_PA12203001)	+= pa12203001.o
 obj-$(CONFIG_RPR0521)		+= rpr0521.o
diff --git a/drivers/iio/light/max44000.c b/drivers/iio/light/max44000.c
new file mode 100644
index 0000000..707cc24
--- /dev/null
+++ b/drivers/iio/light/max44000.c
@@ -0,0 +1,336 @@
+/*
+ * MAX44000 Ambient and Infrared Proximity Sensor
+ *
+ * Copyright (c) 2016, 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.
+ *
+ * Data sheet: https://datasheets.maximintegrated.com/en/ds/MAX44000.pdf
+ *
+ * 7-bit I2C slave address 0x4a
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/i2c.h>
+#include <linux/regmap.h>
+#include <linux/util_macros.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/acpi.h>
+
+#define MAX44000_DRV_NAME		"max44000"
+
+/* Registers in datasheet order */
+#define MAX44000_REG_STATUS		0x00
+#define MAX44000_REG_CFG_MAIN		0x01
+#define MAX44000_REG_CFG_RX		0x02
+#define MAX44000_REG_CFG_TX		0x03
+#define MAX44000_REG_ALS_DATA_HI	0x04
+#define MAX44000_REG_ALS_DATA_LO	0x05
+#define MAX44000_REG_PRX_DATA		0x16
+#define MAX44000_REG_ALS_UPTHR_HI	0x06
+#define MAX44000_REG_ALS_UPTHR_LO	0x07
+#define MAX44000_REG_ALS_LOTHR_HI	0x08
+#define MAX44000_REG_ALS_LOTHR_LO	0x09
+#define MAX44000_REG_PST		0x0a
+#define MAX44000_REG_PRX_IND		0x0b
+#define MAX44000_REG_PRX_THR		0x0c
+#define MAX44000_REG_TRIM_GAIN_GREEN	0x0f
+#define MAX44000_REG_TRIM_GAIN_IR	0x10
+
+/* REG_CFG bits */
+#define MAX44000_CFG_ALSINTE            0x01
+#define MAX44000_CFG_PRXINTE            0x02
+#define MAX44000_CFG_MASK               0x1c
+#define MAX44000_CFG_MODE_SHUTDOWN      0x00
+#define MAX44000_CFG_MODE_ALS_GIR       0x04
+#define MAX44000_CFG_MODE_ALS_G         0x08
+#define MAX44000_CFG_MODE_ALS_IR        0x0c
+#define MAX44000_CFG_MODE_ALS_PRX       0x10
+#define MAX44000_CFG_MODE_PRX           0x14
+#define MAX44000_CFG_TRIM               0x20
+
+/*
+ * Upper 4 bits are not documented but start as 1 on powerup
+ * Setting them to 0 causes proximity to misbehave so set them to 1
+ */
+#define MAX44000_REG_CFG_RX_DEFAULT 0xf0
+
+#define MAX44000_ALSDATA_OVERFLOW	0x4000
+
+#define MAX44000_REGMASK_READABLE ( \
+		(1 << MAX44000_REG_STATUS) | \
+		(1 << MAX44000_REG_CFG_MAIN) | \
+		(1 << MAX44000_REG_CFG_RX) | \
+		(1 << MAX44000_REG_CFG_TX) | \
+		(1 << MAX44000_REG_ALS_DATA_HI) | \
+		(1 << MAX44000_REG_ALS_DATA_LO) | \
+		(1 << MAX44000_REG_PRX_DATA) | \
+		(1 << MAX44000_REG_ALS_UPTHR_HI) | \
+		(1 << MAX44000_REG_ALS_UPTHR_LO) | \
+		(1 << MAX44000_REG_ALS_LOTHR_HI) | \
+		(1 << MAX44000_REG_ALS_LOTHR_LO) | \
+		(1 << MAX44000_REG_PST) | \
+		(1 << MAX44000_REG_PRX_IND) | \
+		(1 << MAX44000_REG_PRX_THR) | \
+		(1 << MAX44000_REG_TRIM_GAIN_GREEN) | \
+		(1 << MAX44000_REG_TRIM_GAIN_IR))
+
+#define MAX44000_REGMASK_WRITEABLE ( \
+		(1 << MAX44000_REG_CFG_MAIN) | \
+		(1 << MAX44000_REG_CFG_RX) | \
+		(1 << MAX44000_REG_CFG_TX) | \
+		(1 << MAX44000_REG_ALS_UPTHR_HI) | \
+		(1 << MAX44000_REG_ALS_UPTHR_LO) | \
+		(1 << MAX44000_REG_ALS_LOTHR_HI) | \
+		(1 << MAX44000_REG_ALS_LOTHR_LO) | \
+		(1 << MAX44000_REG_PST) | \
+		(1 << MAX44000_REG_PRX_IND) | \
+		(1 << MAX44000_REG_PRX_THR) | \
+		(1 << MAX44000_REG_TRIM_GAIN_GREEN) | \
+		(1 << MAX44000_REG_TRIM_GAIN_IR))
+
+#define MAX44000_REGMASK_VOLATILE ( \
+		(1 << MAX44000_REG_STATUS) | \
+		(1 << MAX44000_REG_ALS_DATA_HI) | \
+		(1 << MAX44000_REG_ALS_DATA_LO) | \
+		(1 << MAX44000_REG_PRX_DATA))
+
+struct max44000_data {
+	struct mutex lock;
+	struct regmap *regmap;
+};
+
+/* Default scale is set to the minimum of 0.03125 or 1 / (1 << 5) lux */
+#define MAX44000_ALS_TO_LUX_DEFAULT_FRACTION_LOG2 5
+
+static const struct iio_chan_spec max44000_channels[] = {
+	{
+		.type = IIO_LIGHT,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
+	},
+};
+
+static int max44000_read_alsval(struct max44000_data *data)
+{
+	u16 regval;
+	int ret;
+
+	ret = regmap_bulk_read(data->regmap, MAX44000_REG_ALS_DATA_HI,
+			       &regval, sizeof(regval));
+	if (ret < 0)
+		return ret;
+
+	regval = be16_to_cpu(regval);
+
+	/*
+	 * Overflow is explained on datasheet page 17.
+	 *
+	 * It's a warning that either the G or IR channel has become saturated
+	 * and that the value in the register is likely incorrect.
+	 *
+	 * The recommendation is to change the scale (ALSPGA).
+	 * The driver just returns the max representable value.
+	 */
+	if (regval & MAX44000_ALSDATA_OVERFLOW)
+		return 0x3FFF;
+
+	return regval;
+}
+
+static int max44000_read_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int *val, int *val2, long mask)
+{
+	struct max44000_data *data = iio_priv(indio_dev);
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		switch (chan->type) {
+		case IIO_LIGHT:
+			mutex_lock(&data->lock);
+			ret = max44000_read_alsval(data);
+			mutex_unlock(&data->lock);
+			if (ret < 0)
+				return ret;
+			*val = ret;
+			return IIO_VAL_INT;
+
+		default:
+			return -EINVAL;
+		}
+
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->type) {
+		case IIO_LIGHT:
+			*val = 1;
+			*val2 = MAX44000_ALS_TO_LUX_DEFAULT_FRACTION_LOG2;
+			return IIO_VAL_FRACTIONAL_LOG2;
+
+		default:
+			return -EINVAL;
+		}
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info max44000_info = {
+	.driver_module		= THIS_MODULE,
+	.read_raw		= max44000_read_raw,
+};
+
+static bool max44000_readable_reg(struct device *dev, unsigned int reg)
+{
+	return (1 << reg) & MAX44000_REGMASK_READABLE;
+}
+
+static bool max44000_writeable_reg(struct device *dev, unsigned int reg)
+{
+	return (1 << reg) & MAX44000_REGMASK_WRITEABLE;
+}
+
+static bool max44000_volatile_reg(struct device *dev, unsigned int reg)
+{
+	return (1 << reg) & MAX44000_REGMASK_VOLATILE;
+}
+
+static bool max44000_precious_reg(struct device *dev, unsigned int reg)
+{
+	return reg == MAX44000_REG_STATUS;
+}
+
+/* Datasheet pages 9-10: */
+static const struct reg_default max44000_reg_defaults[] = {
+	{ MAX44000_REG_CFG_MAIN,	0x24 },
+	{ MAX44000_REG_CFG_RX,		MAX44000_REG_CFG_RX_DEFAULT },
+	{ MAX44000_REG_CFG_TX,		0x00 },
+	{ MAX44000_REG_ALS_UPTHR_HI,	0x00 },
+	{ MAX44000_REG_ALS_UPTHR_LO,	0x00 },
+	{ MAX44000_REG_ALS_LOTHR_HI,	0x00 },
+	{ MAX44000_REG_ALS_LOTHR_LO,	0x00 },
+	{ MAX44000_REG_PST,		0x00 },
+	{ MAX44000_REG_PRX_IND,		0x00 },
+	{ MAX44000_REG_PRX_THR,		0x00 },
+	{ MAX44000_REG_TRIM_GAIN_GREEN,	0x80 },
+	{ MAX44000_REG_TRIM_GAIN_IR,	0x80 },
+};
+
+static const struct regmap_config max44000_regmap_config = {
+	.reg_bits	= 8,
+	.val_bits	= 8,
+
+	.max_register	= MAX44000_REG_PRX_DATA,
+	.readable_reg	= max44000_readable_reg,
+	.writeable_reg	= max44000_writeable_reg,
+	.volatile_reg	= max44000_volatile_reg,
+	.precious_reg	= max44000_precious_reg,
+
+	.use_single_rw	= 1,
+	.cache_type	= REGCACHE_FLAT,
+
+	.reg_defaults		= max44000_reg_defaults,
+	.num_reg_defaults	= ARRAY_SIZE(max44000_reg_defaults),
+};
+
+static int max44000_probe(struct i2c_client *client,
+			  const struct i2c_device_id *id)
+{
+	struct max44000_data *data;
+	struct iio_dev *indio_dev;
+	int ret, reg;
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+	data = iio_priv(indio_dev);
+	data->regmap = devm_regmap_init_i2c(client, &max44000_regmap_config);
+	if (IS_ERR(data->regmap)) {
+		dev_err(&client->dev, "regmap_init failed!\n");
+		return PTR_ERR(data->regmap);
+	}
+
+	i2c_set_clientdata(client, indio_dev);
+	mutex_init(&data->lock);
+	indio_dev->dev.parent = &client->dev;
+	indio_dev->info = &max44000_info;
+	indio_dev->name = MAX44000_DRV_NAME;
+	indio_dev->channels = max44000_channels;
+	indio_dev->num_channels = ARRAY_SIZE(max44000_channels);
+
+	/*
+	 * The device doesn't have a reset function so we just clear some
+	 * important bits at probe time to ensure sane operation.
+	 *
+	 * Since we don't support interrupts/events the threshold values are
+	 * not important. We also don't touch trim values.
+	 */
+
+	/* Reset ALS scaling bits */
+	ret = regmap_write(data->regmap, MAX44000_REG_CFG_RX, MAX44000_REG_CFG_RX_DEFAULT);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed to write default CFG_RX: %d\n", ret);
+		return ret;
+	}
+
+	/* Reset CFG bits to ALS-only mode and no interrupts */
+	reg = MAX44000_CFG_TRIM | MAX44000_CFG_MODE_ALS_GIR;
+	ret = regmap_write(data->regmap, MAX44000_REG_CFG_MAIN, reg);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed to write init config: %d\n", ret);
+		return ret;
+	}
+
+	/* Read status at least once to clear any stale interrupt bits. */
+	ret = regmap_read(data->regmap, MAX44000_REG_STATUS, &reg);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed to read init status: %d\n", ret);
+		return ret;
+	}
+
+	return iio_device_register(indio_dev);
+}
+
+static int max44000_remove(struct i2c_client *client)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+
+	iio_device_unregister(indio_dev);
+	return 0;
+}
+
+static const struct i2c_device_id max44000_id[] = {
+	{"max44000", 0},
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, max44000_id);
+
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id max44000_acpi_match[] = {
+	{"MAX44000", 0},
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, max44000_acpi_match);
+#endif
+
+static struct i2c_driver max44000_driver = {
+	.driver = {
+		.name	= MAX44000_DRV_NAME,
+		.acpi_match_table = ACPI_PTR(max44000_acpi_match),
+	},
+	.probe		= max44000_probe,
+	.remove		= max44000_remove,
+	.id_table	= max44000_id,
+};
+
+module_i2c_driver(max44000_driver);
+
+MODULE_AUTHOR("Crestez Dan Leonard <leonard.crestez@intel.com>");
+MODULE_DESCRIPTION("MAX44000 Ambient and Infrared Proximity Sensor");
+MODULE_LICENSE("GPL v2");
-- 
2.5.5

[PATCH v2 2/5] max44000: Initial support for proximity reading

[Crestez Dan Leonard]

The proximity sensor relies on sending pulses to an external IR led and
it is disabled by default on powerup. The driver will enable it with a
default power setting.

Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
---
 drivers/iio/light/max44000.c | 46 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 44 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/light/max44000.c b/drivers/iio/light/max44000.c
index 707cc24..39853ff 100644
--- a/drivers/iio/light/max44000.c
+++ b/drivers/iio/light/max44000.c
@@ -59,6 +59,11 @@
  */
 #define MAX44000_REG_CFG_RX_DEFAULT 0xf0
 
+/* REG_TX bits */
+#define MAX44000_LED_CURRENT_MASK	0xf
+#define MAX44000_LED_CURRENT_MAX	11
+#define MAX44000_LED_CURRENT_DEFAULT	6
+
 #define MAX44000_ALSDATA_OVERFLOW	0x4000
 
 #define MAX44000_REGMASK_READABLE ( \
@@ -111,6 +116,12 @@ static const struct iio_chan_spec max44000_channels[] = {
 	{
 		.type = IIO_LIGHT,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |
+					    BIT(IIO_CHAN_INFO_INT_TIME),
+	},
+	{
+		.type = IIO_PROXIMITY,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
 	},
 };
@@ -142,11 +153,23 @@ static int max44000_read_alsval(struct max44000_data *data)
 	return regval;
 }
 
+static int max44000_write_led_current_raw(struct max44000_data *data, int val)
+{
+	/* Maybe we should clamp the value instead? */
+	if (val < 0 || val > MAX44000_LED_CURRENT_MAX)
+		return -ERANGE;
+	if (val >= 8)
+		val += 4;
+	return regmap_write_bits(data->regmap, MAX44000_REG_CFG_TX,
+				 MAX44000_LED_CURRENT_MASK, val);
+}
+
 static int max44000_read_raw(struct iio_dev *indio_dev,
 			     struct iio_chan_spec const *chan,
 			     int *val, int *val2, long mask)
 {
 	struct max44000_data *data = iio_priv(indio_dev);
+	unsigned int regval;
 	int ret;
 
 	switch (mask) {
@@ -161,6 +184,15 @@ static int max44000_read_raw(struct iio_dev *indio_dev,
 			*val = ret;
 			return IIO_VAL_INT;
 
+		case IIO_PROXIMITY:
+			mutex_lock(&data->lock);
+			ret = regmap_read(data->regmap, MAX44000_REG_PRX_DATA, &regval);
+			mutex_unlock(&data->lock);
+			if (ret < 0)
+				return ret;
+			*val = regval;
+			return IIO_VAL_INT;
+
 		default:
 			return -EINVAL;
 		}
@@ -279,8 +311,18 @@ static int max44000_probe(struct i2c_client *client,
 		return ret;
 	}
 
-	/* Reset CFG bits to ALS-only mode and no interrupts */
-	reg = MAX44000_CFG_TRIM | MAX44000_CFG_MODE_ALS_GIR;
+	/*
+	 * By default the LED pulse used for the proximity sensor is disabled.
+	 * Set a middle value so that we get some sort of valid data by default.
+	 */
+	ret = max44000_write_led_current_raw(data, MAX44000_LED_CURRENT_DEFAULT);
+	if (ret < 0) {
+		dev_err(&client->dev, "failed to write init config: %d\n", ret);
+		return ret;
+	}
+
+	/* Reset CFG bits to ALS_PRX mode which allows easy reading of both values. */
+	reg = MAX44000_CFG_TRIM | MAX44000_CFG_MODE_ALS_PRX;
 	ret = regmap_write(data->regmap, MAX44000_REG_CFG_MAIN, reg);
 	if (ret < 0) {
 		dev_err(&client->dev, "failed to write init config: %d\n", ret);
-- 
2.5.5

[PATCH v2 3/5] max44000: Support controlling LED current output

[Crestez Dan Leonard]

This is exposed as an output channel with "led" as an extend_name.

Other sensors also have support for controlling an external LED. It's
not clear that simply exposing an undecorated output channel is the
correct approach.

Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
---
 drivers/iio/light/max44000.c | 53 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/drivers/iio/light/max44000.c b/drivers/iio/light/max44000.c
index 39853ff..662a45b 100644
--- a/drivers/iio/light/max44000.c
+++ b/drivers/iio/light/max44000.c
@@ -124,6 +124,13 @@ static const struct iio_chan_spec max44000_channels[] = {
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
 	},
+	{
+		.type = IIO_CURRENT,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE),
+		.extend_name = "led",
+		.output = 1,
+	},
 };
 
 static int max44000_read_alsval(struct max44000_data *data)
@@ -164,6 +171,20 @@ static int max44000_write_led_current_raw(struct max44000_data *data, int val)
 				 MAX44000_LED_CURRENT_MASK, val);
 }
 
+static int max44000_read_led_current_raw(struct max44000_data *data)
+{
+	unsigned int regval;
+	int ret;
+
+	ret = regmap_read(data->regmap, MAX44000_REG_CFG_TX, &regval);
+	if (ret < 0)
+		return ret;
+	regval &= MAX44000_LED_CURRENT_MASK;
+	if (regval >= 8)
+		regval -= 4;
+	return regval;
+}
+
 static int max44000_read_raw(struct iio_dev *indio_dev,
 			     struct iio_chan_spec const *chan,
 			     int *val, int *val2, long mask)
@@ -193,12 +214,26 @@ static int max44000_read_raw(struct iio_dev *indio_dev,
 			*val = regval;
 			return IIO_VAL_INT;
 
+		case IIO_CURRENT:
+			mutex_lock(&data->lock);
+			ret = max44000_read_led_current_raw(data);
+			mutex_unlock(&data->lock);
+			if (ret < 0)
+				return ret;
+			*val = ret;
+			return IIO_VAL_INT;
+
 		default:
 			return -EINVAL;
 		}
 
 	case IIO_CHAN_INFO_SCALE:
 		switch (chan->type) {
+		case IIO_CURRENT:
+			/* Output register is in 10s of miliamps */
+			*val = 10;
+			return IIO_VAL_INT;
+
 		case IIO_LIGHT:
 			*val = 1;
 			*val2 = MAX44000_ALS_TO_LUX_DEFAULT_FRACTION_LOG2;
@@ -213,9 +248,27 @@ static int max44000_read_raw(struct iio_dev *indio_dev,
 	}
 }
 
+static int max44000_write_raw(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan,
+			      int val, int val2, long mask)
+{
+	struct max44000_data *data = iio_priv(indio_dev);
+	int ret;
+
+	if (mask == IIO_CHAN_INFO_RAW && chan->type == IIO_CURRENT) {
+		mutex_lock(&data->lock);
+		ret = max44000_write_led_current_raw(data, val);
+		mutex_unlock(&data->lock);
+		return ret;
+	}
+
+	return -EINVAL;
+}
+
 static const struct iio_info max44000_info = {
 	.driver_module		= THIS_MODULE,
 	.read_raw		= max44000_read_raw,
+	.write_raw		= max44000_write_raw,
 };
 
 static bool max44000_readable_reg(struct device *dev, unsigned int reg)
-- 
2.5.5

[PATCH v2 4/5] max44000: Expose ambient sensor scaling

[Crestez Dan Leonard]

This patch exposes ALSTIM as illuminance_integration_time and ALSPGA as
illuminance_scale.

Changing ALSTIM also changes the number of bits available in the data
register. This is handled inside raw value reading because:
* It's very easy to shift a few bits
* It allows SCALE and INT_TIME to be completely independent controls
* Buffer support requires constant scan_type.realbits per-channel

Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
---
 drivers/iio/light/max44000.c | 164 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 160 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/light/max44000.c b/drivers/iio/light/max44000.c
index 662a45b..79a8282 100644
--- a/drivers/iio/light/max44000.c
+++ b/drivers/iio/light/max44000.c
@@ -59,6 +59,12 @@
  */
 #define MAX44000_REG_CFG_RX_DEFAULT 0xf0
 
+/* REG_RX bits */
+#define MAX44000_CFG_RX_ALSTIM_MASK	0x0c
+#define MAX44000_CFG_RX_ALSTIM_SHIFT	2
+#define MAX44000_CFG_RX_ALSPGA_MASK	0x03
+#define MAX44000_CFG_RX_ALSPGA_SHIFT	0
+
 /* REG_TX bits */
 #define MAX44000_LED_CURRENT_MASK	0xf
 #define MAX44000_LED_CURRENT_MAX	11
@@ -112,6 +118,51 @@ struct max44000_data {
 /* Default scale is set to the minimum of 0.03125 or 1 / (1 << 5) lux */
 #define MAX44000_ALS_TO_LUX_DEFAULT_FRACTION_LOG2 5
 
+/* Scale can be multiplied by up to 128x via ALSPGA for measurement gain */
+static const int max44000_alspga_shift[] = {0, 2, 4, 7};
+#define MAX44000_ALSPGA_MAX_SHIFT 7
+
+/*
+ * Scale can be multiplied by up to 64x via ALSTIM because of lost resolution
+ *
+ * This scaling factor is hidden from userspace and instead accounted for when
+ * reading raw values from the device.
+ *
+ * This makes it possible to cleanly expose ALSPGA as IIO_CHAN_INFO_SCALE and
+ * ALSTIM as IIO_CHAN_INFO_INT_TIME without the values affecting each other.
+ *
+ * Handling this internally is also required for buffer support because the
+ * channel's scan_type can't be modified dynamically.
+ */
+static const int max44000_alstim_shift[] = {0, 2, 4, 6};
+#define MAX44000_ALSTIM_SHIFT(alstim) (2 * (alstim))
+
+/* Available integration times with pretty manual alignment: */
+static const int max44000_int_time_avail_ns_array[] = {
+	   100000000,
+	    25000000,
+	     6250000,
+	     1562500,
+};
+static const char max44000_int_time_avail_str[] =
+	"0.100 "
+	"0.025 "
+	"0.00625 "
+	"0.001625";
+
+/* Available scales (internal to ulux) with pretty manual alignment: */
+static const int max44000_scale_avail_ulux_array[] = {
+	    31250,
+	   125000,
+	   500000,
+	  4000000,
+};
+static const char max44000_scale_avail_str[] =
+	"0.03125 "
+	"0.125 "
+	"0.5 "
+	 "4";
+
 static const struct iio_chan_spec max44000_channels[] = {
 	{
 		.type = IIO_LIGHT,
@@ -133,15 +184,54 @@ static const struct iio_chan_spec max44000_channels[] = {
 	},
 };
 
+static int max44000_read_alstim(struct max44000_data *data)
+{
+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(data->regmap, MAX44000_REG_CFG_RX, &val);
+	if (ret < 0)
+		return ret;
+	return (val & MAX44000_CFG_RX_ALSTIM_MASK) >> MAX44000_CFG_RX_ALSTIM_SHIFT;
+}
+
+static int max44000_write_alstim(struct max44000_data *data, int val)
+{
+	return regmap_write_bits(data->regmap, MAX44000_REG_CFG_RX,
+				 MAX44000_CFG_RX_ALSTIM_MASK,
+				 val << MAX44000_CFG_RX_ALSTIM_SHIFT);
+}
+
+static int max44000_read_alspga(struct max44000_data *data)
+{
+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(data->regmap, MAX44000_REG_CFG_RX, &val);
+	if (ret < 0)
+		return ret;
+	return (val & MAX44000_CFG_RX_ALSPGA_MASK) >> MAX44000_CFG_RX_ALSPGA_SHIFT;
+}
+
+static int max44000_write_alspga(struct max44000_data *data, int val)
+{
+	return regmap_write_bits(data->regmap, MAX44000_REG_CFG_RX,
+				 MAX44000_CFG_RX_ALSPGA_MASK,
+				 val << MAX44000_CFG_RX_ALSPGA_SHIFT);
+}
+
 static int max44000_read_alsval(struct max44000_data *data)
 {
 	u16 regval;
-	int ret;
+	int alstim, ret;
 
 	ret = regmap_bulk_read(data->regmap, MAX44000_REG_ALS_DATA_HI,
 			       &regval, sizeof(regval));
 	if (ret < 0)
 		return ret;
+	alstim = ret = max44000_read_alstim(data);
+	if (ret < 0)
+		return ret;
 
 	regval = be16_to_cpu(regval);
 
@@ -157,7 +247,7 @@ static int max44000_read_alsval(struct max44000_data *data)
 	if (regval & MAX44000_ALSDATA_OVERFLOW)
 		return 0x3FFF;
 
-	return regval;
+	return regval << MAX44000_ALSTIM_SHIFT(alstim);
 }
 
 static int max44000_write_led_current_raw(struct max44000_data *data, int val)
@@ -190,6 +280,7 @@ static int max44000_read_raw(struct iio_dev *indio_dev,
 			     int *val, int *val2, long mask)
 {
 	struct max44000_data *data = iio_priv(indio_dev);
+	int alstim, alspga;
 	unsigned int regval;
 	int ret;
 
@@ -235,14 +326,34 @@ static int max44000_read_raw(struct iio_dev *indio_dev,
 			return IIO_VAL_INT;
 
 		case IIO_LIGHT:
-			*val = 1;
-			*val2 = MAX44000_ALS_TO_LUX_DEFAULT_FRACTION_LOG2;
+			mutex_lock(&data->lock);
+			alspga = ret = max44000_read_alspga(data);
+			mutex_unlock(&data->lock);
+			if (ret < 0)
+				return ret;
+
+			/* Avoid negative shifts */
+			*val = (1 << MAX44000_ALSPGA_MAX_SHIFT);
+			*val2 = MAX44000_ALS_TO_LUX_DEFAULT_FRACTION_LOG2
+					+ MAX44000_ALSPGA_MAX_SHIFT
+					- max44000_alspga_shift[alspga];
 			return IIO_VAL_FRACTIONAL_LOG2;
 
 		default:
 			return -EINVAL;
 		}
 
+	case IIO_CHAN_INFO_INT_TIME:
+		mutex_lock(&data->lock);
+		alstim = ret = max44000_read_alstim(data);
+		mutex_unlock(&data->lock);
+
+		if (ret < 0)
+			return ret;
+		*val = 0;
+		*val2 = max44000_int_time_avail_ns_array[alstim];
+		return IIO_VAL_INT_PLUS_NANO;
+
 	default:
 		return -EINVAL;
 	}
@@ -260,15 +371,60 @@ static int max44000_write_raw(struct iio_dev *indio_dev,
 		ret = max44000_write_led_current_raw(data, val);
 		mutex_unlock(&data->lock);
 		return ret;
+	} else if (mask == IIO_CHAN_INFO_INT_TIME && chan->type == IIO_LIGHT) {
+		s64 valns = val * NSEC_PER_SEC + val2;
+		int alstim = find_closest_descending(valns,
+				max44000_int_time_avail_ns_array,
+				ARRAY_SIZE(max44000_int_time_avail_ns_array));
+		mutex_lock(&data->lock);
+		ret = max44000_write_alstim(data, alstim);
+		mutex_unlock(&data->lock);
+		return ret;
+	} else if (mask == IIO_CHAN_INFO_SCALE && chan->type == IIO_LIGHT) {
+		s64 valus = val * USEC_PER_SEC + val2;
+		int alspga = find_closest(valus,
+				max44000_scale_avail_ulux_array,
+				ARRAY_SIZE(max44000_scale_avail_ulux_array));
+		mutex_lock(&data->lock);
+		ret = max44000_write_alspga(data, alspga);
+		mutex_unlock(&data->lock);
+		return ret;
 	}
 
 	return -EINVAL;
 }
 
+static int max44000_write_raw_get_fmt(struct iio_dev *indio_dev,
+				      struct iio_chan_spec const *chan,
+				      long mask)
+{
+	if (mask == IIO_CHAN_INFO_INT_TIME && chan->type == IIO_LIGHT)
+		return IIO_VAL_INT_PLUS_NANO;
+	else if (mask == IIO_CHAN_INFO_SCALE && chan->type == IIO_LIGHT)
+		return IIO_VAL_INT_PLUS_MICRO;
+	else
+		return IIO_VAL_INT;
+}
+
+static IIO_CONST_ATTR(illuminance_integration_time_available, max44000_int_time_avail_str);
+static IIO_CONST_ATTR(illuminance_scale_available, max44000_scale_avail_str);
+
+static struct attribute *max44000_attributes[] = {
+	&iio_const_attr_illuminance_integration_time_available.dev_attr.attr,
+	&iio_const_attr_illuminance_scale_available.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group max44000_attribute_group = {
+	.attrs = max44000_attributes,
+};
+
 static const struct iio_info max44000_info = {
 	.driver_module		= THIS_MODULE,
 	.read_raw		= max44000_read_raw,
 	.write_raw		= max44000_write_raw,
+	.write_raw_get_fmt	= max44000_write_raw_get_fmt,
+	.attrs			= &max44000_attribute_group,
 };
 
 static bool max44000_readable_reg(struct device *dev, unsigned int reg)
-- 
2.5.5

[PATCH v2 5/5] max44000: Initial triggered buffer support

[Crestez Dan Leonard]

Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
---
 drivers/iio/light/max44000.c | 62 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/drivers/iio/light/max44000.c b/drivers/iio/light/max44000.c
index 79a8282..48955df 100644
--- a/drivers/iio/light/max44000.c
+++ b/drivers/iio/light/max44000.c
@@ -19,6 +19,9 @@
 #include <linux/util_macros.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
 #include <linux/acpi.h>
 
 #define MAX44000_DRV_NAME		"max44000"
@@ -163,24 +166,41 @@ static const char max44000_scale_avail_str[] =
 	"0.5 "
 	 "4";
 
+#define MAX44000_SCAN_INDEX_ALS 0
+#define MAX44000_SCAN_INDEX_PRX 1
+
 static const struct iio_chan_spec max44000_channels[] = {
 	{
 		.type = IIO_LIGHT,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |
 					    BIT(IIO_CHAN_INFO_INT_TIME),
+		.scan_index = MAX44000_SCAN_INDEX_ALS,
+		.scan_type = {
+			.sign		= 'u',
+			.realbits	= 14,
+			.storagebits	= 16,
+		}
 	},
 	{
 		.type = IIO_PROXIMITY,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
 		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
+		.scan_index = MAX44000_SCAN_INDEX_PRX,
+		.scan_type = {
+			.sign		= 'u',
+			.realbits	= 8,
+			.storagebits	= 16,
+		}
 	},
+	IIO_CHAN_SOFT_TIMESTAMP(2),
 	{
 		.type = IIO_CURRENT,
 		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
 				      BIT(IIO_CHAN_INFO_SCALE),
 		.extend_name = "led",
 		.output = 1,
+		.scan_index = -1,
 	},
 };
 
@@ -480,6 +500,41 @@ static const struct regmap_config max44000_regmap_config = {
 	.num_reg_defaults	= ARRAY_SIZE(max44000_reg_defaults),
 };
 
+static irqreturn_t max44000_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct max44000_data *data = iio_priv(indio_dev);
+	u16 buf[8]; /* 2x u16 + padding + 8 bytes timestamp */
+	int index = 0;
+	unsigned int regval;
+	int ret;
+
+	mutex_lock(&data->lock);
+	if (*indio_dev->active_scan_mask & (1 << MAX44000_SCAN_INDEX_ALS)) {
+		ret = max44000_read_alsval(data);
+		if (ret < 0)
+			goto out_unlock;
+		buf[index++] = ret;
+	}
+	if (*indio_dev->active_scan_mask & (1 << MAX44000_SCAN_INDEX_PRX)) {
+		ret = regmap_read(data->regmap, MAX44000_REG_PRX_DATA, &regval);
+		if (ret < 0)
+			goto out_unlock;
+		buf[index] = regval;
+	}
+	mutex_unlock(&data->lock);
+
+	iio_push_to_buffers_with_timestamp(indio_dev, buf, iio_get_time_ns());
+	iio_trigger_notify_done(indio_dev->trig);
+	return IRQ_HANDLED;
+
+out_unlock:
+	mutex_unlock(&data->lock);
+	iio_trigger_notify_done(indio_dev->trig);
+	return IRQ_HANDLED;
+}
+
 static int max44000_probe(struct i2c_client *client,
 			  const struct i2c_device_id *id)
 {
@@ -545,6 +600,12 @@ static int max44000_probe(struct i2c_client *client,
 		return ret;
 	}
 
+	ret = iio_triggered_buffer_setup(indio_dev, NULL, max44000_trigger_handler, NULL);
+	if (ret < 0) {
+		dev_err(&client->dev, "iio triggered buffer setup failed\n");
+		return ret;
+	}
+
 	return iio_device_register(indio_dev);
 }
 
@@ -553,6 +614,7 @@ static int max44000_remove(struct i2c_client *client)
 	struct iio_dev *indio_dev = i2c_get_clientdata(client);
 
 	iio_device_unregister(indio_dev);
+	iio_triggered_buffer_cleanup(indio_dev);
 	return 0;
 }
 
-- 
2.5.5

Re: [PATCH v2 1/5] max44000: Initial support

[Jonathan Cameron]

On 11/04/16 19:52, Crestez Dan Leonard wrote:
> This just adds support for reporting illuminance with default settings.
> 
> Important default registers are written on probe because the device
> otherwise lacks a reset function.
> 
> Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
cc'ing Mark Brown for regcache flat discussion..   Mark I don't suppose you
have any docs anywhere I can point people at on this.  It comes round rather
to frequently for my taste and I can't immediately find any references in
the tree or code to explain what REGCACHE_FLAT is for...  I've cc'd your
previous email on the subject, but it's not really documentation.


> ---
>  drivers/iio/light/Kconfig    |  11 ++
>  drivers/iio/light/Makefile   |   1 +
>  drivers/iio/light/max44000.c | 336 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 348 insertions(+)
>  create mode 100644 drivers/iio/light/max44000.c
> 
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index cfd3df8..b4ab65b 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -223,6 +223,17 @@ config LTR501
>  	 This driver can also be built as a module.  If so, the module
>           will be called ltr501.
>  
> +config MAX44000
> +	tristate "MAX44000 Ambient and Infrared Proximity Sensor"
> +	depends on I2C
> +	select REGMAP_I2C
> +	help
> +	 Say Y here if you want to build support for Maxim Integrated's
> +	 MAX44000 ambient and infrared proximity sensor device.
> +
> +	 To compile this driver as a module, choose M here:
> +	 the module will be called max44000.
> +
>  config OPT3001
>  	tristate "Texas Instruments OPT3001 Light Sensor"
>  	depends on I2C
> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> index b2c3105..c77f27f 100644
> --- a/drivers/iio/light/Makefile
> +++ b/drivers/iio/light/Makefile
> @@ -20,6 +20,7 @@ obj-$(CONFIG_ISL29125)		+= isl29125.o
>  obj-$(CONFIG_JSA1212)		+= jsa1212.o
>  obj-$(CONFIG_SENSORS_LM3533)	+= lm3533-als.o
>  obj-$(CONFIG_LTR501)		+= ltr501.o
> +obj-$(CONFIG_MAX44000)		+= max44000.o
>  obj-$(CONFIG_OPT3001)		+= opt3001.o
>  obj-$(CONFIG_PA12203001)	+= pa12203001.o
>  obj-$(CONFIG_RPR0521)		+= rpr0521.o
> diff --git a/drivers/iio/light/max44000.c b/drivers/iio/light/max44000.c
> new file mode 100644
> index 0000000..707cc24
> --- /dev/null
> +++ b/drivers/iio/light/max44000.c
> @@ -0,0 +1,336 @@
> +/*
> + * MAX44000 Ambient and Infrared Proximity Sensor
> + *
> + * Copyright (c) 2016, 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.
> + *
> + * Data sheet: https://datasheets.maximintegrated.com/en/ds/MAX44000.pdf
> + *
> + * 7-bit I2C slave address 0x4a
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/i2c.h>
> +#include <linux/regmap.h>
> +#include <linux/util_macros.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/acpi.h>
> +
> +#define MAX44000_DRV_NAME		"max44000"
> +
> +/* Registers in datasheet order */
> +#define MAX44000_REG_STATUS		0x00
> +#define MAX44000_REG_CFG_MAIN		0x01
> +#define MAX44000_REG_CFG_RX		0x02
> +#define MAX44000_REG_CFG_TX		0x03
> +#define MAX44000_REG_ALS_DATA_HI	0x04
> +#define MAX44000_REG_ALS_DATA_LO	0x05
> +#define MAX44000_REG_PRX_DATA		0x16
> +#define MAX44000_REG_ALS_UPTHR_HI	0x06
> +#define MAX44000_REG_ALS_UPTHR_LO	0x07
> +#define MAX44000_REG_ALS_LOTHR_HI	0x08
> +#define MAX44000_REG_ALS_LOTHR_LO	0x09
> +#define MAX44000_REG_PST		0x0a
> +#define MAX44000_REG_PRX_IND		0x0b
> +#define MAX44000_REG_PRX_THR		0x0c
> +#define MAX44000_REG_TRIM_GAIN_GREEN	0x0f
> +#define MAX44000_REG_TRIM_GAIN_IR	0x10
> +
> +/* REG_CFG bits */
> +#define MAX44000_CFG_ALSINTE            0x01
> +#define MAX44000_CFG_PRXINTE            0x02
> +#define MAX44000_CFG_MASK               0x1c
> +#define MAX44000_CFG_MODE_SHUTDOWN      0x00
> +#define MAX44000_CFG_MODE_ALS_GIR       0x04
> +#define MAX44000_CFG_MODE_ALS_G         0x08
> +#define MAX44000_CFG_MODE_ALS_IR        0x0c
> +#define MAX44000_CFG_MODE_ALS_PRX       0x10
> +#define MAX44000_CFG_MODE_PRX           0x14
> +#define MAX44000_CFG_TRIM               0x20
> +
> +/*
> + * Upper 4 bits are not documented but start as 1 on powerup
> + * Setting them to 0 causes proximity to misbehave so set them to 1
> + */
> +#define MAX44000_REG_CFG_RX_DEFAULT 0xf0
> +
> +#define MAX44000_ALSDATA_OVERFLOW	0x4000
> +
> +#define MAX44000_REGMASK_READABLE ( \
Whilst an improvement I'd still prefer the nice clean
switch statement in the function.  Let the compiler do the magic for
you.
> +		(1 << MAX44000_REG_STATUS) | \
> +		(1 << MAX44000_REG_CFG_MAIN) | \
> +		(1 << MAX44000_REG_CFG_RX) | \
> +		(1 << MAX44000_REG_CFG_TX) | \
> +		(1 << MAX44000_REG_ALS_DATA_HI) | \
> +		(1 << MAX44000_REG_ALS_DATA_LO) | \
> +		(1 << MAX44000_REG_PRX_DATA) | \
> +		(1 << MAX44000_REG_ALS_UPTHR_HI) | \
> +		(1 << MAX44000_REG_ALS_UPTHR_LO) | \
> +		(1 << MAX44000_REG_ALS_LOTHR_HI) | \
> +		(1 << MAX44000_REG_ALS_LOTHR_LO) | \
> +		(1 << MAX44000_REG_PST) | \
> +		(1 << MAX44000_REG_PRX_IND) | \
> +		(1 << MAX44000_REG_PRX_THR) | \
> +		(1 << MAX44000_REG_TRIM_GAIN_GREEN) | \
> +		(1 << MAX44000_REG_TRIM_GAIN_IR))
> +
> +#define MAX44000_REGMASK_WRITEABLE ( \
> +		(1 << MAX44000_REG_CFG_MAIN) | \
> +		(1 << MAX44000_REG_CFG_RX) | \
> +		(1 << MAX44000_REG_CFG_TX) | \
> +		(1 << MAX44000_REG_ALS_UPTHR_HI) | \
> +		(1 << MAX44000_REG_ALS_UPTHR_LO) | \
> +		(1 << MAX44000_REG_ALS_LOTHR_HI) | \
> +		(1 << MAX44000_REG_ALS_LOTHR_LO) | \
> +		(1 << MAX44000_REG_PST) | \
> +		(1 << MAX44000_REG_PRX_IND) | \
> +		(1 << MAX44000_REG_PRX_THR) | \
> +		(1 << MAX44000_REG_TRIM_GAIN_GREEN) | \
> +		(1 << MAX44000_REG_TRIM_GAIN_IR))
> +
> +#define MAX44000_REGMASK_VOLATILE ( \
> +		(1 << MAX44000_REG_STATUS) | \
> +		(1 << MAX44000_REG_ALS_DATA_HI) | \
> +		(1 << MAX44000_REG_ALS_DATA_LO) | \
> +		(1 << MAX44000_REG_PRX_DATA))
> +
> +struct max44000_data {
> +	struct mutex lock;
> +	struct regmap *regmap;
> +};
> +
> +/* Default scale is set to the minimum of 0.03125 or 1 / (1 << 5) lux */
> +#define MAX44000_ALS_TO_LUX_DEFAULT_FRACTION_LOG2 5
> +
> +static const struct iio_chan_spec max44000_channels[] = {
> +	{
> +		.type = IIO_LIGHT,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> +	},
> +};
> +
> +static int max44000_read_alsval(struct max44000_data *data)
> +{
> +	u16 regval;
> +	int ret;
> +
> +	ret = regmap_bulk_read(data->regmap, MAX44000_REG_ALS_DATA_HI,
> +			       &regval, sizeof(regval));
> +	if (ret < 0)
> +		return ret;
> +
> +	regval = be16_to_cpu(regval);
> +
> +	/*
> +	 * Overflow is explained on datasheet page 17.
> +	 *
> +	 * It's a warning that either the G or IR channel has become saturated
> +	 * and that the value in the register is likely incorrect.
> +	 *
> +	 * The recommendation is to change the scale (ALSPGA).
> +	 * The driver just returns the max representable value.
> +	 */
Good comment. Thanks for adding it.
> +	if (regval & MAX44000_ALSDATA_OVERFLOW)
> +		return 0x3FFF;
> +
> +	return regval;
> +}
> +
> +static int max44000_read_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int *val, int *val2, long mask)
> +{
> +	struct max44000_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		switch (chan->type) {
> +		case IIO_LIGHT:
> +			mutex_lock(&data->lock);
> +			ret = max44000_read_alsval(data);
> +			mutex_unlock(&data->lock);
> +			if (ret < 0)
> +				return ret;
> +			*val = ret;
> +			return IIO_VAL_INT;
> +
> +		default:
> +			return -EINVAL;
> +		}
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->type) {
> +		case IIO_LIGHT:
> +			*val = 1;
On this one I'm not sure the define adds anything given it really is a
numerical value.. I'd just put it inline here.
> +			*val2 = MAX44000_ALS_TO_LUX_DEFAULT_FRACTION_LOG2;
> +			return IIO_VAL_FRACTIONAL_LOG2;
> +
> +		default:
> +			return -EINVAL;
> +		}
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_info max44000_info = {
> +	.driver_module		= THIS_MODULE,
> +	.read_raw		= max44000_read_raw,
> +};
> +
> +static bool max44000_readable_reg(struct device *dev, unsigned int reg)
> +{
> +	return (1 << reg) & MAX44000_REGMASK_READABLE;
> +}
> +
> +static bool max44000_writeable_reg(struct device *dev, unsigned int reg)
> +{
> +	return (1 << reg) & MAX44000_REGMASK_WRITEABLE;
> +}
> +
> +static bool max44000_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	return (1 << reg) & MAX44000_REGMASK_VOLATILE;
> +}
> +
> +static bool max44000_precious_reg(struct device *dev, unsigned int reg)
> +{
> +	return reg == MAX44000_REG_STATUS;
> +}
> +
> +/* Datasheet pages 9-10: */
> +static const struct reg_default max44000_reg_defaults[] = {
> +	{ MAX44000_REG_CFG_MAIN,	0x24 },
This must be broken down into defines showing what the various bits are
doing.

> +	{ MAX44000_REG_CFG_RX,		MAX44000_REG_CFG_RX_DEFAULT },
> +	{ MAX44000_REG_CFG_TX,		0x00 },
> +	{ MAX44000_REG_ALS_UPTHR_HI,	0x00 },
> +	{ MAX44000_REG_ALS_UPTHR_LO,	0x00 },
> +	{ MAX44000_REG_ALS_LOTHR_HI,	0x00 },
> +	{ MAX44000_REG_ALS_LOTHR_LO,	0x00 },
> +	{ MAX44000_REG_PST,		0x00 },
> +	{ MAX44000_REG_PRX_IND,		0x00 },
> +	{ MAX44000_REG_PRX_THR,		0x00 },
> +	{ MAX44000_REG_TRIM_GAIN_GREEN,	0x80 },
> +	{ MAX44000_REG_TRIM_GAIN_IR,	0x80 },
> +};
> +
> +static const struct regmap_config max44000_regmap_config = {
> +	.reg_bits	= 8,
> +	.val_bits	= 8,
> +
> +	.max_register	= MAX44000_REG_PRX_DATA,
> +	.readable_reg	= max44000_readable_reg,
> +	.writeable_reg	= max44000_writeable_reg,
> +	.volatile_reg	= max44000_volatile_reg,
> +	.precious_reg	= max44000_precious_reg,
> +
> +	.use_single_rw	= 1,
> +	.cache_type	= REGCACHE_FLAT,

Please read:
http://www.spinics.net/lists/linux-iio/msg22996.html
You definitely want REGCACHE_RBTREE which behaves quite differently wrt
to what happens on startup of the cache.
> +
If we aren't assuming particular values, what is the purpose of providing
defaults?  They are probably wrong..
> +	.reg_defaults		= max44000_reg_defaults,
> +	.num_reg_defaults	= ARRAY_SIZE(max44000_reg_defaults),
> +};
> +
> +static int max44000_probe(struct i2c_client *client,
> +			  const struct i2c_device_id *id)
> +{
> +	struct max44000_data *data;
> +	struct iio_dev *indio_dev;
> +	int ret, reg;
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +	data = iio_priv(indio_dev);
> +	data->regmap = devm_regmap_init_i2c(client, &max44000_regmap_config);
> +	if (IS_ERR(data->regmap)) {
> +		dev_err(&client->dev, "regmap_init failed!\n");
> +		return PTR_ERR(data->regmap);
> +	}
> +
> +	i2c_set_clientdata(client, indio_dev);
> +	mutex_init(&data->lock);
> +	indio_dev->dev.parent = &client->dev;
> +	indio_dev->info = &max44000_info;
> +	indio_dev->name = MAX44000_DRV_NAME;
> +	indio_dev->channels = max44000_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(max44000_channels);
> +
> +	/*
> +	 * The device doesn't have a reset function so we just clear some
> +	 * important bits at probe time to ensure sane operation.
> +	 *
> +	 * Since we don't support interrupts/events the threshold values are
> +	 * not important. We also don't touch trim values.
> +	 */
> +
> +	/* Reset ALS scaling bits */
> +	ret = regmap_write(data->regmap, MAX44000_REG_CFG_RX, MAX44000_REG_CFG_RX_DEFAULT);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "failed to write default CFG_RX: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* Reset CFG bits to ALS-only mode and no interrupts */
> +	reg = MAX44000_CFG_TRIM | MAX44000_CFG_MODE_ALS_GIR;
> +	ret = regmap_write(data->regmap, MAX44000_REG_CFG_MAIN, reg);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "failed to write init config: %d\n", ret);
> +		return ret;
> +	}
> +
I prefer this careful code making it clear what you are doing on setting the
defaults to the previous wholesale version.

> +	/* Read status at least once to clear any stale interrupt bits. */
> +	ret = regmap_read(data->regmap, MAX44000_REG_STATUS, &reg);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "failed to read init status: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return iio_device_register(indio_dev);
> +}
> +
> +static int max44000_remove(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +
> +	iio_device_unregister(indio_dev);
blank line here ideally (nitpick on the day :)
> +	return 0;
> +}
> +
> +static const struct i2c_device_id max44000_id[] = {
> +	{"max44000", 0},
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, max44000_id);
> +
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id max44000_acpi_match[] = {
> +	{"MAX44000", 0},
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(acpi, max44000_acpi_match);
> +#endif
> +
> +static struct i2c_driver max44000_driver = {
> +	.driver = {
> +		.name	= MAX44000_DRV_NAME,
> +		.acpi_match_table = ACPI_PTR(max44000_acpi_match),
> +	},
> +	.probe		= max44000_probe,
> +	.remove		= max44000_remove,
> +	.id_table	= max44000_id,
> +};
> +
> +module_i2c_driver(max44000_driver);
> +
> +MODULE_AUTHOR("Crestez Dan Leonard <leonard.crestez@intel.com>");
> +MODULE_DESCRIPTION("MAX44000 Ambient and Infrared Proximity Sensor");
> +MODULE_LICENSE("GPL v2");
> 

Re: [PATCH v2 2/5] max44000: Initial support for proximity reading

[Jonathan Cameron]

On 11/04/16 19:52, Crestez Dan Leonard wrote:
> The proximity sensor relies on sending pulses to an external IR led and
> it is disabled by default on powerup. The driver will enable it with a
> default power setting.
> 
> Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
> ---
>  drivers/iio/light/max44000.c | 46 ++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 44 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/iio/light/max44000.c b/drivers/iio/light/max44000.c
> index 707cc24..39853ff 100644
> --- a/drivers/iio/light/max44000.c
> +++ b/drivers/iio/light/max44000.c
> @@ -59,6 +59,11 @@
>   */
>  #define MAX44000_REG_CFG_RX_DEFAULT 0xf0
>  
> +/* REG_TX bits */
> +#define MAX44000_LED_CURRENT_MASK	0xf
> +#define MAX44000_LED_CURRENT_MAX	11
> +#define MAX44000_LED_CURRENT_DEFAULT	6
> +
>  #define MAX44000_ALSDATA_OVERFLOW	0x4000
>  
>  #define MAX44000_REGMASK_READABLE ( \
> @@ -111,6 +116,12 @@ static const struct iio_chan_spec max44000_channels[] = {
>  	{
>  		.type = IIO_LIGHT,
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
> +		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |
> +					    BIT(IIO_CHAN_INFO_INT_TIME),
Where did INT_TIME for the ambient light channel sneak in from?
Astray from patch 4?
> +	},
> +	{
> +		.type = IIO_PROXIMITY,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>  		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
>  	},
>  };
> @@ -142,11 +153,23 @@ static int max44000_read_alsval(struct max44000_data *data)
>  	return regval;
>  }
>  
> +static int max44000_write_led_current_raw(struct max44000_data *data, int val)
> +{
> +	/* Maybe we should clamp the value instead? */
> +	if (val < 0 || val > MAX44000_LED_CURRENT_MAX)
> +		return -ERANGE;
> +	if (val >= 8)
> +		val += 4;
> +	return regmap_write_bits(data->regmap, MAX44000_REG_CFG_TX,
> +				 MAX44000_LED_CURRENT_MASK, val);
> +}
> +
>  static int max44000_read_raw(struct iio_dev *indio_dev,
>  			     struct iio_chan_spec const *chan,
>  			     int *val, int *val2, long mask)
>  {
>  	struct max44000_data *data = iio_priv(indio_dev);
> +	unsigned int regval;
>  	int ret;
>  
>  	switch (mask) {
> @@ -161,6 +184,15 @@ static int max44000_read_raw(struct iio_dev *indio_dev,
>  			*val = ret;
>  			return IIO_VAL_INT;
>  
> +		case IIO_PROXIMITY:
> +			mutex_lock(&data->lock);
> +			ret = regmap_read(data->regmap, MAX44000_REG_PRX_DATA, &regval);
> +			mutex_unlock(&data->lock);
> +			if (ret < 0)
> +				return ret;
> +			*val = regval;
> +			return IIO_VAL_INT;
> +
>  		default:
>  			return -EINVAL;
>  		}
> @@ -279,8 +311,18 @@ static int max44000_probe(struct i2c_client *client,
>  		return ret;
>  	}
>  
> -	/* Reset CFG bits to ALS-only mode and no interrupts */
> -	reg = MAX44000_CFG_TRIM | MAX44000_CFG_MODE_ALS_GIR;
> +	/*
> +	 * By default the LED pulse used for the proximity sensor is disabled.
> +	 * Set a middle value so that we get some sort of valid data by default.
> +	 */
> +	ret = max44000_write_led_current_raw(data, MAX44000_LED_CURRENT_DEFAULT);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "failed to write init config: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* Reset CFG bits to ALS_PRX mode which allows easy reading of both values. */
> +	reg = MAX44000_CFG_TRIM | MAX44000_CFG_MODE_ALS_PRX;
>  	ret = regmap_write(data->regmap, MAX44000_REG_CFG_MAIN, reg);
>  	if (ret < 0) {
>  		dev_err(&client->dev, "failed to write init config: %d\n", ret);
> 

Re: [PATCH v2 3/5] max44000: Support controlling LED current output

[Jonathan Cameron]

On 11/04/16 19:52, Crestez Dan Leonard wrote:
> This is exposed as an output channel with "led" as an extend_name.
> 
> Other sensors also have support for controlling an external LED. It's
> not clear that simply exposing an undecorated output channel is the
> correct approach.
Agreed that this is still unclear.  Lets go with this for now, we can always
add an additional interface to control it if we come to a firm conclusion on
this in the future.

This patch looks good to me.
> 
> Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
> ---
>  drivers/iio/light/max44000.c | 53 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 
> diff --git a/drivers/iio/light/max44000.c b/drivers/iio/light/max44000.c
> index 39853ff..662a45b 100644
> --- a/drivers/iio/light/max44000.c
> +++ b/drivers/iio/light/max44000.c
> @@ -124,6 +124,13 @@ static const struct iio_chan_spec max44000_channels[] = {
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>  		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
>  	},
> +	{
> +		.type = IIO_CURRENT,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE),
> +		.extend_name = "led",
> +		.output = 1,
> +	},
>  };
>  
>  static int max44000_read_alsval(struct max44000_data *data)
> @@ -164,6 +171,20 @@ static int max44000_write_led_current_raw(struct max44000_data *data, int val)
>  				 MAX44000_LED_CURRENT_MASK, val);
>  }
>  
> +static int max44000_read_led_current_raw(struct max44000_data *data)
> +{
> +	unsigned int regval;
> +	int ret;
> +
> +	ret = regmap_read(data->regmap, MAX44000_REG_CFG_TX, &regval);
> +	if (ret < 0)
> +		return ret;
> +	regval &= MAX44000_LED_CURRENT_MASK;
> +	if (regval >= 8)
> +		regval -= 4;
> +	return regval;
> +}
> +
>  static int max44000_read_raw(struct iio_dev *indio_dev,
>  			     struct iio_chan_spec const *chan,
>  			     int *val, int *val2, long mask)
> @@ -193,12 +214,26 @@ static int max44000_read_raw(struct iio_dev *indio_dev,
>  			*val = regval;
>  			return IIO_VAL_INT;
>  
> +		case IIO_CURRENT:
> +			mutex_lock(&data->lock);
> +			ret = max44000_read_led_current_raw(data);
> +			mutex_unlock(&data->lock);
> +			if (ret < 0)
> +				return ret;
> +			*val = ret;
> +			return IIO_VAL_INT;
> +
>  		default:
>  			return -EINVAL;
>  		}
>  
>  	case IIO_CHAN_INFO_SCALE:
>  		switch (chan->type) {
> +		case IIO_CURRENT:
> +			/* Output register is in 10s of miliamps */
> +			*val = 10;
> +			return IIO_VAL_INT;
> +
>  		case IIO_LIGHT:
>  			*val = 1;
>  			*val2 = MAX44000_ALS_TO_LUX_DEFAULT_FRACTION_LOG2;
> @@ -213,9 +248,27 @@ static int max44000_read_raw(struct iio_dev *indio_dev,
>  	}
>  }
>  
> +static int max44000_write_raw(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan,
> +			      int val, int val2, long mask)
> +{
> +	struct max44000_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	if (mask == IIO_CHAN_INFO_RAW && chan->type == IIO_CURRENT) {
> +		mutex_lock(&data->lock);
> +		ret = max44000_write_led_current_raw(data, val);
> +		mutex_unlock(&data->lock);
> +		return ret;
> +	}
> +
> +	return -EINVAL;
> +}
> +
>  static const struct iio_info max44000_info = {
>  	.driver_module		= THIS_MODULE,
>  	.read_raw		= max44000_read_raw,
> +	.write_raw		= max44000_write_raw,
>  };
>  
>  static bool max44000_readable_reg(struct device *dev, unsigned int reg)
> 

Re: [PATCH v2 4/5] max44000: Expose ambient sensor scaling

[Jonathan Cameron]

On 11/04/16 19:52, Crestez Dan Leonard wrote:
> This patch exposes ALSTIM as illuminance_integration_time and ALSPGA as
> illuminance_scale.
> 
> Changing ALSTIM also changes the number of bits available in the data
> register. This is handled inside raw value reading because:
> * It's very easy to shift a few bits
> * It allows SCALE and INT_TIME to be completely independent controls
> * Buffer support requires constant scan_type.realbits per-channel
> 
> Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
Looks good to me.
> ---
>  drivers/iio/light/max44000.c | 164 +++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 160 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/light/max44000.c b/drivers/iio/light/max44000.c
> index 662a45b..79a8282 100644
> --- a/drivers/iio/light/max44000.c
> +++ b/drivers/iio/light/max44000.c
> @@ -59,6 +59,12 @@
>   */
>  #define MAX44000_REG_CFG_RX_DEFAULT 0xf0
>  
> +/* REG_RX bits */
> +#define MAX44000_CFG_RX_ALSTIM_MASK	0x0c
> +#define MAX44000_CFG_RX_ALSTIM_SHIFT	2
> +#define MAX44000_CFG_RX_ALSPGA_MASK	0x03
> +#define MAX44000_CFG_RX_ALSPGA_SHIFT	0
> +
>  /* REG_TX bits */
>  #define MAX44000_LED_CURRENT_MASK	0xf
>  #define MAX44000_LED_CURRENT_MAX	11
> @@ -112,6 +118,51 @@ struct max44000_data {
>  /* Default scale is set to the minimum of 0.03125 or 1 / (1 << 5) lux */
>  #define MAX44000_ALS_TO_LUX_DEFAULT_FRACTION_LOG2 5
>  
> +/* Scale can be multiplied by up to 128x via ALSPGA for measurement gain */
> +static const int max44000_alspga_shift[] = {0, 2, 4, 7};
> +#define MAX44000_ALSPGA_MAX_SHIFT 7
> +
> +/*
> + * Scale can be multiplied by up to 64x via ALSTIM because of lost resolution
> + *
> + * This scaling factor is hidden from userspace and instead accounted for when
> + * reading raw values from the device.
> + *
> + * This makes it possible to cleanly expose ALSPGA as IIO_CHAN_INFO_SCALE and
> + * ALSTIM as IIO_CHAN_INFO_INT_TIME without the values affecting each other.
> + *
> + * Handling this internally is also required for buffer support because the
> + * channel's scan_type can't be modified dynamically.
> + */
> +static const int max44000_alstim_shift[] = {0, 2, 4, 6};
> +#define MAX44000_ALSTIM_SHIFT(alstim) (2 * (alstim))
> +
> +/* Available integration times with pretty manual alignment: */
> +static const int max44000_int_time_avail_ns_array[] = {
> +	   100000000,
> +	    25000000,
> +	     6250000,
> +	     1562500,
> +};
> +static const char max44000_int_time_avail_str[] =
> +	"0.100 "
> +	"0.025 "
> +	"0.00625 "
> +	"0.001625";
> +
> +/* Available scales (internal to ulux) with pretty manual alignment: */
> +static const int max44000_scale_avail_ulux_array[] = {
> +	    31250,
> +	   125000,
> +	   500000,
> +	  4000000,
> +};
> +static const char max44000_scale_avail_str[] =
> +	"0.03125 "
> +	"0.125 "
> +	"0.5 "
> +	 "4";
> +
>  static const struct iio_chan_spec max44000_channels[] = {
>  	{
>  		.type = IIO_LIGHT,
> @@ -133,15 +184,54 @@ static const struct iio_chan_spec max44000_channels[] = {
>  	},
>  };
>  
> +static int max44000_read_alstim(struct max44000_data *data)
> +{
> +	unsigned int val;
> +	int ret;
> +
> +	ret = regmap_read(data->regmap, MAX44000_REG_CFG_RX, &val);
> +	if (ret < 0)
> +		return ret;
> +	return (val & MAX44000_CFG_RX_ALSTIM_MASK) >> MAX44000_CFG_RX_ALSTIM_SHIFT;
> +}
> +
> +static int max44000_write_alstim(struct max44000_data *data, int val)
> +{
> +	return regmap_write_bits(data->regmap, MAX44000_REG_CFG_RX,
> +				 MAX44000_CFG_RX_ALSTIM_MASK,
> +				 val << MAX44000_CFG_RX_ALSTIM_SHIFT);
> +}
> +
> +static int max44000_read_alspga(struct max44000_data *data)
> +{
> +	unsigned int val;
> +	int ret;
> +
> +	ret = regmap_read(data->regmap, MAX44000_REG_CFG_RX, &val);
> +	if (ret < 0)
> +		return ret;
> +	return (val & MAX44000_CFG_RX_ALSPGA_MASK) >> MAX44000_CFG_RX_ALSPGA_SHIFT;
> +}
> +
> +static int max44000_write_alspga(struct max44000_data *data, int val)
> +{
> +	return regmap_write_bits(data->regmap, MAX44000_REG_CFG_RX,
> +				 MAX44000_CFG_RX_ALSPGA_MASK,
> +				 val << MAX44000_CFG_RX_ALSPGA_SHIFT);
> +}
> +
>  static int max44000_read_alsval(struct max44000_data *data)
>  {
>  	u16 regval;
> -	int ret;
> +	int alstim, ret;
>  
>  	ret = regmap_bulk_read(data->regmap, MAX44000_REG_ALS_DATA_HI,
>  			       &regval, sizeof(regval));
>  	if (ret < 0)
>  		return ret;
> +	alstim = ret = max44000_read_alstim(data);
> +	if (ret < 0)
> +		return ret;
>  
>  	regval = be16_to_cpu(regval);
>  
> @@ -157,7 +247,7 @@ static int max44000_read_alsval(struct max44000_data *data)
>  	if (regval & MAX44000_ALSDATA_OVERFLOW)
>  		return 0x3FFF;
>  
> -	return regval;
> +	return regval << MAX44000_ALSTIM_SHIFT(alstim);
>  }
>  
>  static int max44000_write_led_current_raw(struct max44000_data *data, int val)
> @@ -190,6 +280,7 @@ static int max44000_read_raw(struct iio_dev *indio_dev,
>  			     int *val, int *val2, long mask)
>  {
>  	struct max44000_data *data = iio_priv(indio_dev);
> +	int alstim, alspga;
>  	unsigned int regval;
>  	int ret;
>  
> @@ -235,14 +326,34 @@ static int max44000_read_raw(struct iio_dev *indio_dev,
>  			return IIO_VAL_INT;
>  
>  		case IIO_LIGHT:
> -			*val = 1;
> -			*val2 = MAX44000_ALS_TO_LUX_DEFAULT_FRACTION_LOG2;
> +			mutex_lock(&data->lock);
> +			alspga = ret = max44000_read_alspga(data);
> +			mutex_unlock(&data->lock);
> +			if (ret < 0)
> +				return ret;
> +
> +			/* Avoid negative shifts */
> +			*val = (1 << MAX44000_ALSPGA_MAX_SHIFT);
> +			*val2 = MAX44000_ALS_TO_LUX_DEFAULT_FRACTION_LOG2
> +					+ MAX44000_ALSPGA_MAX_SHIFT
> +					- max44000_alspga_shift[alspga];
>  			return IIO_VAL_FRACTIONAL_LOG2;
>  
>  		default:
>  			return -EINVAL;
>  		}
>  
> +	case IIO_CHAN_INFO_INT_TIME:
> +		mutex_lock(&data->lock);
> +		alstim = ret = max44000_read_alstim(data);
> +		mutex_unlock(&data->lock);
> +
> +		if (ret < 0)
> +			return ret;
> +		*val = 0;
> +		*val2 = max44000_int_time_avail_ns_array[alstim];
> +		return IIO_VAL_INT_PLUS_NANO;
> +
>  	default:
>  		return -EINVAL;
>  	}
> @@ -260,15 +371,60 @@ static int max44000_write_raw(struct iio_dev *indio_dev,
>  		ret = max44000_write_led_current_raw(data, val);
>  		mutex_unlock(&data->lock);
>  		return ret;
> +	} else if (mask == IIO_CHAN_INFO_INT_TIME && chan->type == IIO_LIGHT) {
> +		s64 valns = val * NSEC_PER_SEC + val2;
> +		int alstim = find_closest_descending(valns,
> +				max44000_int_time_avail_ns_array,
> +				ARRAY_SIZE(max44000_int_time_avail_ns_array));
> +		mutex_lock(&data->lock);
> +		ret = max44000_write_alstim(data, alstim);
> +		mutex_unlock(&data->lock);
> +		return ret;
> +	} else if (mask == IIO_CHAN_INFO_SCALE && chan->type == IIO_LIGHT) {
> +		s64 valus = val * USEC_PER_SEC + val2;
> +		int alspga = find_closest(valus,
> +				max44000_scale_avail_ulux_array,
> +				ARRAY_SIZE(max44000_scale_avail_ulux_array));
> +		mutex_lock(&data->lock);
> +		ret = max44000_write_alspga(data, alspga);
> +		mutex_unlock(&data->lock);
> +		return ret;
>  	}
>  
>  	return -EINVAL;
>  }
>  
> +static int max44000_write_raw_get_fmt(struct iio_dev *indio_dev,
> +				      struct iio_chan_spec const *chan,
> +				      long mask)
> +{
> +	if (mask == IIO_CHAN_INFO_INT_TIME && chan->type == IIO_LIGHT)
> +		return IIO_VAL_INT_PLUS_NANO;
> +	else if (mask == IIO_CHAN_INFO_SCALE && chan->type == IIO_LIGHT)
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	else
> +		return IIO_VAL_INT;
> +}
> +
> +static IIO_CONST_ATTR(illuminance_integration_time_available, max44000_int_time_avail_str);
> +static IIO_CONST_ATTR(illuminance_scale_available, max44000_scale_avail_str);
> +
> +static struct attribute *max44000_attributes[] = {
> +	&iio_const_attr_illuminance_integration_time_available.dev_attr.attr,
> +	&iio_const_attr_illuminance_scale_available.dev_attr.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group max44000_attribute_group = {
> +	.attrs = max44000_attributes,
> +};
> +
>  static const struct iio_info max44000_info = {
>  	.driver_module		= THIS_MODULE,
>  	.read_raw		= max44000_read_raw,
>  	.write_raw		= max44000_write_raw,
> +	.write_raw_get_fmt	= max44000_write_raw_get_fmt,
> +	.attrs			= &max44000_attribute_group,
>  };
>  
>  static bool max44000_readable_reg(struct device *dev, unsigned int reg)
> 

Re: [PATCH v2 5/5] max44000: Initial triggered buffer support

[Jonathan Cameron]

On 11/04/16 19:52, Crestez Dan Leonard wrote:
> Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
One trivial comment inline.
> ---
>  drivers/iio/light/max44000.c | 62 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 62 insertions(+)
> 
> diff --git a/drivers/iio/light/max44000.c b/drivers/iio/light/max44000.c
> index 79a8282..48955df 100644
> --- a/drivers/iio/light/max44000.c
> +++ b/drivers/iio/light/max44000.c
> @@ -19,6 +19,9 @@
>  #include <linux/util_macros.h>
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
>  #include <linux/acpi.h>
>  
>  #define MAX44000_DRV_NAME		"max44000"
> @@ -163,24 +166,41 @@ static const char max44000_scale_avail_str[] =
>  	"0.5 "
>  	 "4";
>  
> +#define MAX44000_SCAN_INDEX_ALS 0
> +#define MAX44000_SCAN_INDEX_PRX 1
> +
>  static const struct iio_chan_spec max44000_channels[] = {
>  	{
>  		.type = IIO_LIGHT,
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>  		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |
>  					    BIT(IIO_CHAN_INFO_INT_TIME),
> +		.scan_index = MAX44000_SCAN_INDEX_ALS,
> +		.scan_type = {
> +			.sign		= 'u',
> +			.realbits	= 14,
> +			.storagebits	= 16,
> +		}
>  	},
>  	{
>  		.type = IIO_PROXIMITY,
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
>  		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
> +		.scan_index = MAX44000_SCAN_INDEX_PRX,
> +		.scan_type = {
> +			.sign		= 'u',
> +			.realbits	= 8,
> +			.storagebits	= 16,
> +		}
>  	},
> +	IIO_CHAN_SOFT_TIMESTAMP(2),
>  	{
>  		.type = IIO_CURRENT,
>  		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>  				      BIT(IIO_CHAN_INFO_SCALE),
>  		.extend_name = "led",
>  		.output = 1,
> +		.scan_index = -1,
>  	},
>  };
>  
> @@ -480,6 +500,41 @@ static const struct regmap_config max44000_regmap_config = {
>  	.num_reg_defaults	= ARRAY_SIZE(max44000_reg_defaults),
>  };
>  
> +static irqreturn_t max44000_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct max44000_data *data = iio_priv(indio_dev);
> +	u16 buf[8]; /* 2x u16 + padding + 8 bytes timestamp */
> +	int index = 0;
> +	unsigned int regval;
> +	int ret;
> +
> +	mutex_lock(&data->lock);
Slightly nicer to use the BIT macro.
> +	if (*indio_dev->active_scan_mask & (1 << MAX44000_SCAN_INDEX_ALS)) {
> +		ret = max44000_read_alsval(data);
> +		if (ret < 0)
> +			goto out_unlock;
> +		buf[index++] = ret;
> +	}
> +	if (*indio_dev->active_scan_mask & (1 << MAX44000_SCAN_INDEX_PRX)) {
> +		ret = regmap_read(data->regmap, MAX44000_REG_PRX_DATA, &regval);
> +		if (ret < 0)
> +			goto out_unlock;
> +		buf[index] = regval;
> +	}
> +	mutex_unlock(&data->lock);
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, buf, iio_get_time_ns());
> +	iio_trigger_notify_done(indio_dev->trig);
> +	return IRQ_HANDLED;
> +
> +out_unlock:
> +	mutex_unlock(&data->lock);
> +	iio_trigger_notify_done(indio_dev->trig);
> +	return IRQ_HANDLED;
> +}
> +
>  static int max44000_probe(struct i2c_client *client,
>  			  const struct i2c_device_id *id)
>  {
> @@ -545,6 +600,12 @@ static int max44000_probe(struct i2c_client *client,
>  		return ret;
>  	}
>  
> +	ret = iio_triggered_buffer_setup(indio_dev, NULL, max44000_trigger_handler, NULL);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "iio triggered buffer setup failed\n");
> +		return ret;
> +	}
> +
>  	return iio_device_register(indio_dev);
>  }
>  
> @@ -553,6 +614,7 @@ static int max44000_remove(struct i2c_client *client)
>  	struct iio_dev *indio_dev = i2c_get_clientdata(client);
>  
>  	iio_device_unregister(indio_dev);
> +	iio_triggered_buffer_cleanup(indio_dev);
>  	return 0;
>  }
>  
>