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

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

[Crestez Dan Leonard]

The driver supports reporting illuminance and proximity (via raw reads or
software-triggered) buffers and controling the scaling factors.

There is no support for power management, events or reporting the IR/green
channels separately. The device is always set in the ALS/PROX mode in order to
report all data.

Crestez Dan Leonard (5):
  max44000: Initial commit
  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 | 627 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 639 insertions(+)
 create mode 100644 drivers/iio/light/max44000.c

-- 
2.8.0.rc3

[PATCH 1/5] max44000: Initial commit

[Crestez Dan Leonard]

This just adds support for reporting illuminance with default settings.

All 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 | 295 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 307 insertions(+)
 create mode 100644 drivers/iio/light/max44000.c

diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index cfd3df8..42c15c1 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -320,4 +320,15 @@ config VCNL4000
 	 To compile this driver as a module, choose M here: the
 	 module will be called vcnl4000.
 
+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.
+
 endmenu
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index b2c3105..14b2f36 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -30,3 +30,4 @@ obj-$(CONFIG_TCS3472)		+= tcs3472.o
 obj-$(CONFIG_TSL4531)		+= tsl4531.o
 obj-$(CONFIG_US5182D)		+= us5182d.o
 obj-$(CONFIG_VCNL4000)		+= vcnl4000.o
+obj-$(CONFIG_MAX44000)		+= max44000.o
diff --git a/drivers/iio/light/max44000.c b/drivers/iio/light/max44000.c
new file mode 100644
index 0000000..7c12153
--- /dev/null
+++ b/drivers/iio/light/max44000.c
@@ -0,0 +1,295 @@
+/*
+ * 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
+
+#define MAX44000_ALSDATA_OVERFLOW	0x4000
+
+#define MAX44000_REGMASK_READABLE	0x419fff
+#define MAX44000_REGMASK_WRITEABLE	0x019fce
+#define MAX44000_REGMASK_VOLATILE	0x400031
+
+struct max44000_data {
+	struct mutex lock;
+	struct i2c_client *client;
+	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 inline int max44000_read_alsval(struct max44000_data *data)
+{
+	u16 regval;
+	int ret;
+
+	regval = 0;
+	ret = regmap_bulk_read(data->regmap, MAX44000_REG_ALS_DATA_HI, &regval, 2);
+	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 },
+	/* 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
+	 */
+	{ MAX44000_REG_CFG_RX,		0xf0 },
+	{ 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_force_write_defaults(struct max44000_data *data)
+{
+	int i, ret;
+
+	for (i = 0; i < ARRAY_SIZE(max44000_reg_defaults); ++i) {
+		ret = regmap_write(data->regmap,
+				   max44000_reg_defaults[i].reg,
+				   max44000_reg_defaults[i].def);
+		if (ret)
+			return ret;
+	}
+	return 0;
+}
+
+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);
+	data->client = client;
+	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);
+
+	/* Read status at least once to clear the power-on-reset bit. */
+	ret = regmap_read(data->regmap, MAX44000_REG_STATUS, &reg);
+	if (ret < 0) {
+		dev_err(&data->client->dev, "failed to read init status: %d\n", ret);
+		return ret;
+	}
+
+	/* The device has no reset command, write defaults explicitly. */
+	ret = max44000_force_write_defaults(data);
+	if (ret < 0) {
+		dev_err(&data->client->dev, "failed to write defaults: %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_OF
+static const struct of_device_id max44000_of_match[] = {
+	{ .compatible = "maxim,max44000" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, max44000_of_match);
+#endif
+
+#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,
+		.of_match_table = of_match_ptr(max44000_of_match),
+		.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.8.0.rc3

[PATCH 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 | 61 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/drivers/iio/light/max44000.c b/drivers/iio/light/max44000.c
index 7c12153..10a0fe8 100644
--- a/drivers/iio/light/max44000.c
+++ b/drivers/iio/light/max44000.c
@@ -41,6 +41,23 @@
 #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
+
+/* 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	0x419fff
@@ -60,6 +77,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),
 	},
 };
@@ -90,11 +113,23 @@ static inline int max44000_read_alsval(struct max44000_data *data)
 	return regval;
 }
 
+static inline 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) {
@@ -109,6 +144,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;
 		}
@@ -244,6 +288,23 @@ static int max44000_probe(struct i2c_client *client,
 		return ret;
 	}
 
+	/* 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(&data->client->dev, "failed to write init config: %d\n", ret);
+		return ret;
+	}
+
+	/* By default set in 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(&data->client->dev, "failed to write init config: %d\n", ret);
+		return ret;
+	}
+
 	return iio_device_register(indio_dev);
 }
 
-- 
2.8.0.rc3

[PATCH 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 10a0fe8..1dc10b9 100644
--- a/drivers/iio/light/max44000.c
+++ b/drivers/iio/light/max44000.c
@@ -85,6 +85,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 inline int max44000_read_alsval(struct max44000_data *data)
@@ -124,6 +131,20 @@ static inline int max44000_write_led_current_raw(struct max44000_data *data, int
 				 MAX44000_LED_CURRENT_MASK, val);
 }
 
+static inline 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)
@@ -153,12 +174,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;
@@ -173,9 +208,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.8.0.rc3

[PATCH 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 | 163 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 159 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/light/max44000.c b/drivers/iio/light/max44000.c
index 1dc10b9..e479c53 100644
--- a/drivers/iio/light/max44000.c
+++ b/drivers/iio/light/max44000.c
@@ -53,6 +53,12 @@
 #define MAX44000_CFG_MODE_PRX		0x14
 #define MAX44000_CFG_TRIM		0x20
 
+/* 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
@@ -73,6 +79,50 @@ 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.
+ *
+ * Hanlding 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,
@@ -94,15 +144,54 @@ static const struct iio_chan_spec max44000_channels[] = {
 	},
 };
 
+static inline 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 inline 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 inline 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 inline 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 inline int max44000_read_alsval(struct max44000_data *data)
 {
 	u16 regval;
-	int ret;
+	int alstim, ret;
 
 	regval = 0;
 	ret = regmap_bulk_read(data->regmap, MAX44000_REG_ALS_DATA_HI, &regval, 2);
 	if (ret < 0)
 		return ret;
+	alstim = ret = max44000_read_alstim(data);
+	if (ret < 0)
+		return ret;
 
 	regval = be16_to_cpu(regval);
 
@@ -117,7 +206,7 @@ static inline int max44000_read_alsval(struct max44000_data *data)
 	if (regval & MAX44000_ALSDATA_OVERFLOW)
 		return 0x3FFF;
 
-	return regval;
+	return regval << MAX44000_ALSTIM_SHIFT(alstim);
 }
 
 static inline int max44000_write_led_current_raw(struct max44000_data *data, int val)
@@ -150,6 +239,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;
 
@@ -195,14 +285,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;
 	}
@@ -220,15 +330,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.8.0.rc3

[PATCH 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 | 63 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/drivers/iio/light/max44000.c b/drivers/iio/light/max44000.c
index e479c53..7b1f8bc 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"
@@ -123,24 +126,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	= 8,
+		}
 	},
+	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,
 	},
 };
 
@@ -456,6 +476,42 @@ static int max44000_force_write_defaults(struct max44000_data *data)
 	return 0;
 }
 
+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[indio_dev->scan_bytes / 2];
+	u8 *pos = (u8 *)buf;
+	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;
+		*((u16 *)pos) = ret;
+		pos += 2;
+	}
+	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;
+		*pos = 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)
 {
@@ -513,6 +569,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);
 }
 
@@ -521,6 +583,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.8.0.rc3

Re: [PATCH 1/5] max44000: Initial commit

[Peter Meerwald-Stadler]

> This just adds support for reporting illuminance with default settings.
> 
> All default registers are written on probe because the device otherwise
> lacks a reset function.

comments below
 
> 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 | 295 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 307 insertions(+)
>  create mode 100644 drivers/iio/light/max44000.c
> 
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index cfd3df8..42c15c1 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -320,4 +320,15 @@ config VCNL4000
>  	 To compile this driver as a module, choose M here: the
>  	 module will be called vcnl4000.
>  
> +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.
> +
>  endmenu
> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
> index b2c3105..14b2f36 100644
> --- a/drivers/iio/light/Makefile
> +++ b/drivers/iio/light/Makefile
> @@ -30,3 +30,4 @@ obj-$(CONFIG_TCS3472)		+= tcs3472.o
>  obj-$(CONFIG_TSL4531)		+= tsl4531.o
>  obj-$(CONFIG_US5182D)		+= us5182d.o
>  obj-$(CONFIG_VCNL4000)		+= vcnl4000.o
> +obj-$(CONFIG_MAX44000)		+= max44000.o

alphabetical order please

> diff --git a/drivers/iio/light/max44000.c b/drivers/iio/light/max44000.c
> new file mode 100644
> index 0000000..7c12153
> --- /dev/null
> +++ b/drivers/iio/light/max44000.c
> @@ -0,0 +1,295 @@
> +/*
> + * 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
> +
> +#define MAX44000_ALSDATA_OVERFLOW	0x4000
> +
> +#define MAX44000_REGMASK_READABLE	0x419fff
> +#define MAX44000_REGMASK_WRITEABLE	0x019fce
> +#define MAX44000_REGMASK_VOLATILE	0x400031
> +
> +struct max44000_data {
> +	struct mutex lock;
> +	struct i2c_client *client;
> +	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 inline int max44000_read_alsval(struct max44000_data *data)

drop inline, let the compiler figure out if inlining is beneficial

> +{
> +	u16 regval;
> +	int ret;
> +
> +	regval = 0;

needed?

> +	ret = regmap_bulk_read(data->regmap, MAX44000_REG_ALS_DATA_HI, &regval, 2);

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 },
> +	/* 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
> +	 */
> +	{ MAX44000_REG_CFG_RX,		0xf0 },
> +	{ 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_force_write_defaults(struct max44000_data *data)
> +{
> +	int i, ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(max44000_reg_defaults); ++i) {
> +		ret = regmap_write(data->regmap,
> +				   max44000_reg_defaults[i].reg,
> +				   max44000_reg_defaults[i].def);
> +		if (ret)
> +			return ret;
> +	}
> +	return 0;
> +}
> +
> +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);
> +	data->client = client;
> +	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);
> +
> +	/* Read status at least once to clear the power-on-reset bit. */
> +	ret = regmap_read(data->regmap, MAX44000_REG_STATUS, &reg);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "failed to read init status: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* The device has no reset command, write defaults explicitly. */
> +	ret = max44000_force_write_defaults(data);
> +	if (ret < 0) {
> +		dev_err(&data->client->dev, "failed to write defaults: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return iio_device_register(indio_dev);

devm_ would work here to make _remove obsolete

> +}
> +
> +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_OF
> +static const struct of_device_id max44000_of_match[] = {
> +	{ .compatible = "maxim,max44000" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, max44000_of_match);
> +#endif
> +
> +#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,
> +		.of_match_table = of_match_ptr(max44000_of_match),
> +		.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");
> 

-- 

Peter Meerwald-Stadler
+43-664-2444418 (mobile)

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

[Peter Meerwald-Stadler]

comments below

> Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
> ---
>  drivers/iio/light/max44000.c | 63 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
> 
> diff --git a/drivers/iio/light/max44000.c b/drivers/iio/light/max44000.c
> index e479c53..7b1f8bc 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"
> @@ -123,24 +126,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	= 8,

code would get simpler if 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,
>  	},
>  };
>  
> @@ -456,6 +476,42 @@ static int max44000_force_write_defaults(struct max44000_data *data)
>  	return 0;
>  }
>  
> +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[indio_dev->scan_bytes / 2];
> +	u8 *pos = (u8 *)buf;

int i = 0;

> +	unsigned int regval;
> +	int ret;
> +
> +	mutex_lock(&data->lock);
> +	if (*indio_dev->active_scan_mask & (1 << MAX44000_SCAN_INDEX_ALS)) {

BIT(MAX44000_SCAN_INDEX_ALS)

> +		ret = max44000_read_alsval(data);
> +		if (ret < 0)
> +			goto out_unlock;
> +		*((u16 *)pos) = ret;

buf[i++] = ret;

> +		pos += 2;
> +	}
> +	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[i++] = retval;

> +		*pos = regval;
> +	}
> +	mutex_unlock(&data->lock);
> +
> +	iio_push_to_buffers_with_timestamp(indio_dev, buf, iio_get_time_ns());

moving iio_push_to_buffers_with_timestamp() before mutex_unlock() would 
allow simpler code (not sure if it's worth)

> +	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)
>  {
> @@ -513,6 +569,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);

no devm_ possible anymore :-)

>  }
>  
> @@ -521,6 +583,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;
>  }
>  
> 

-- 

Peter Meerwald-Stadler
+43-664-2444418 (mobile)

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1990 bytes --]

Hi Crestez,

[auto build test WARNING on iio/togreg]
[also build test WARNING on v4.6-rc2 next-20160407]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Crestez-Dan-Leonard/Support-for-max44000-Ambient-and-Infrared-Proximity-Sensor/20160408-003121
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
config: s390-allyesconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=s390 

All warnings (new ones prefixed by >>):

   drivers/iio/light/max44000.c: In function 'max44000_trigger_handler':
>> drivers/iio/light/max44000.c:513:1: warning: 'max44000_trigger_handler' uses dynamic stack allocation
    }
    ^

vim +/max44000_trigger_handler +513 drivers/iio/light/max44000.c

   497		if (*indio_dev->active_scan_mask & (1 << MAX44000_SCAN_INDEX_PRX)) {
   498			ret = regmap_read(data->regmap, MAX44000_REG_PRX_DATA, &regval);
   499			if (ret < 0)
   500				goto out_unlock;
   501			*pos = regval;
   502		}
   503		mutex_unlock(&data->lock);
   504	
   505		iio_push_to_buffers_with_timestamp(indio_dev, buf, iio_get_time_ns());
   506		iio_trigger_notify_done(indio_dev->trig);
   507		return IRQ_HANDLED;
   508	
   509	out_unlock:
   510		mutex_unlock(&data->lock);
   511		iio_trigger_notify_done(indio_dev->trig);
   512		return IRQ_HANDLED;
 > 513	}
   514	
   515	static int max44000_probe(struct i2c_client *client,
   516				  const struct i2c_device_id *id)
   517	{
   518		struct max44000_data *data;
   519		struct iio_dev *indio_dev;
   520		int ret, reg;
   521	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 40183 bytes --]

Re: [PATCH 1/5] max44000: Initial commit

[Jonathan Cameron]

On 07/04/16 20:48, Peter Meerwald-Stadler wrote:
> 
>> This just adds support for reporting illuminance with default settings.
>>
>> All default registers are written on probe because the device otherwise
>> lacks a reset function.
> 
> comments below
Mostly fine, but a few corners need cleaning up.

Also, I'm not keep on the brute force write everything.  The driver should
cope with any values in those registers and deal with refreshing the
cache etc so that it can do so.  Writing a bunch of defaults is rather a
brittle approach.

Jonathan
>  
>> 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 | 295 +++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 307 insertions(+)
>>  create mode 100644 drivers/iio/light/max44000.c
>>
>> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
>> index cfd3df8..42c15c1 100644
>> --- a/drivers/iio/light/Kconfig
>> +++ b/drivers/iio/light/Kconfig
>> @@ -320,4 +320,15 @@ config VCNL4000
>>  	 To compile this driver as a module, choose M here: the
>>  	 module will be called vcnl4000.
>>  
>> +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.
>> +
>>  endmenu
>> diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
>> index b2c3105..14b2f36 100644
>> --- a/drivers/iio/light/Makefile
>> +++ b/drivers/iio/light/Makefile
>> @@ -30,3 +30,4 @@ obj-$(CONFIG_TCS3472)		+= tcs3472.o
>>  obj-$(CONFIG_TSL4531)		+= tsl4531.o
>>  obj-$(CONFIG_US5182D)		+= us5182d.o
>>  obj-$(CONFIG_VCNL4000)		+= vcnl4000.o
>> +obj-$(CONFIG_MAX44000)		+= max44000.o
> 
> alphabetical order please
> 
>> diff --git a/drivers/iio/light/max44000.c b/drivers/iio/light/max44000.c
>> new file mode 100644
>> index 0000000..7c12153
>> --- /dev/null
>> +++ b/drivers/iio/light/max44000.c
>> @@ -0,0 +1,295 @@
>> +/*
>> + * 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
>> +
>> +#define MAX44000_ALSDATA_OVERFLOW	0x4000
>> +
>> +#define MAX44000_REGMASK_READABLE	0x419fff
>> +#define MAX44000_REGMASK_WRITEABLE	0x019fce
>> +#define MAX44000_REGMASK_VOLATILE	0x400031
These are horrible!  Do it as a switch over the relevant
registers in the functions below...  Much easier to read / debug.
>> +
>> +struct max44000_data {
>> +	struct mutex lock;
>> +	struct i2c_client *client;
This client pointer isn't used outside probe and remove where it is easily
available anyway.  Hence don't keep a copy in here.
>> +	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 inline int max44000_read_alsval(struct max44000_data *data)
> 
> drop inline, let the compiler figure out if inlining is beneficial
> 
>> +{
>> +	u16 regval;
>> +	int ret;
>> +
>> +	regval = 0;
> 
> needed?
> 
>> +	ret = regmap_bulk_read(data->regmap, MAX44000_REG_ALS_DATA_HI, &regval, 2);
> 
> 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;
See above.  This is a really nasty and hard to review way of doing this.
switch (reg) {
       REG1:
       REG2:
       REG3:
	  return true;
       default:
          return false;

may be more code, but it's easy to tell if it is right.
>> +}
>> +
>> +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 },
>> +	/* Upper 4 bits are not documented but start as 1 on powerup
Multiline comment syntax please.
>> +	 * Setting them to 0 causes proximity to misbehave so set them to 1
>> +	 */
>> +	{ MAX44000_REG_CFG_RX,		0xf0 },
>> +	{ 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,
This always seems like a good idea, but tends to cause issues.
FLAT is really only meant for very high performance devices, you
are probably better with something else here.  If you are doing this
deliberately to make the below writes actually occur, then please
add a comment here.
>> +
>> +	.reg_defaults		= max44000_reg_defaults,
>> +	.num_reg_defaults	= ARRAY_SIZE(max44000_reg_defaults),
>> +};
>> +
>> +static int max44000_force_write_defaults(struct max44000_data *data)
>> +{
>> +	int i, ret;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(max44000_reg_defaults); ++i) {
>> +		ret = regmap_write(data->regmap,
>> +				   max44000_reg_defaults[i].reg,
>> +				   max44000_reg_defaults[i].def);
Silly question, but if the cached value matches the values you are trying
to write here will this work?

There is a regcache_mark_dirty call that will ensure all registers in the
cache are read..

If you then need any particular values they should be explicitly written
on the assumption you have no idea what the state is.  Brute force writing
all the defaults is nasty and doesn't give any information as to what
is happening.

>> +		if (ret)
>> +			return ret;
>> +	}
>> +	return 0;
>> +}
>> +
>> +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);
>> +	data->client = client;
>> +	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);
>> +
>> +	/* Read status at least once to clear the power-on-reset bit. */
>> +	ret = regmap_read(data->regmap, MAX44000_REG_STATUS, &reg);
>> +	if (ret < 0) {
>> +		dev_err(&data->client->dev, "failed to read init status: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	/* The device has no reset command, write defaults explicitly. */
>> +	ret = max44000_force_write_defaults(data);
>> +	if (ret < 0) {
>> +		dev_err(&data->client->dev, "failed to write defaults: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	return iio_device_register(indio_dev);
> 
> devm_ would work here to make _remove obsolete
> 
>> +}
>> +
>> +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_OF
>> +static const struct of_device_id max44000_of_match[] = {
>> +	{ .compatible = "maxim,max44000" },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, max44000_of_match);
>> +#endif
>> +
>> +#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,
>> +		.of_match_table = of_match_ptr(max44000_of_match),
>> +		.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 2/5] max44000: Initial support for proximity reading

[Jonathan Cameron]

On 07/04/16 17:21, 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>
One trivial point inline...
> ---
>  drivers/iio/light/max44000.c | 61 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 61 insertions(+)
> 
> diff --git a/drivers/iio/light/max44000.c b/drivers/iio/light/max44000.c
> index 7c12153..10a0fe8 100644
> --- a/drivers/iio/light/max44000.c
> +++ b/drivers/iio/light/max44000.c
> @@ -41,6 +41,23 @@
>  #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
> +
> +/* 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	0x419fff
> @@ -60,6 +77,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),
>  	},
>  };
> @@ -90,11 +113,23 @@ static inline int max44000_read_alsval(struct max44000_data *data)
>  	return regval;
>  }
>  
> +static inline 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) {
> @@ -109,6 +144,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;
>  		}
> @@ -244,6 +288,23 @@ static int max44000_probe(struct i2c_client *client,
>  		return ret;
>  	}
>  
> +	/* By default the LED pulse used for the proximity sensor is disabled.
Please run checkpatch.pl on these.  It would probably have complained about
the multiline comment syntax here.
> +	 * 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(&data->client->dev, "failed to write init config: %d\n", ret);
> +		return ret;
> +	}
> +
> +	/* By default set in 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(&data->client->dev, "failed to write init config: %d\n", ret);
> +		return ret;
> +	}
> +
>  	return iio_device_register(indio_dev);
>  }
>  
> 

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

[Jonathan Cameron]

On 07/04/16 17:21, 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 less than ideal. Other suggestions welcome.
It gets complicated as some devices have multiple LEDs and they aren't
necessarily directly associated with a particular sensor.

> 
> 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 10a0fe8..1dc10b9 100644
> --- a/drivers/iio/light/max44000.c
> +++ b/drivers/iio/light/max44000.c
> @@ -85,6 +85,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 inline int max44000_read_alsval(struct max44000_data *data)
> @@ -124,6 +131,20 @@ static inline int max44000_write_led_current_raw(struct max44000_data *data, int
>  				 MAX44000_LED_CURRENT_MASK, val);
>  }
>  
> +static inline 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)
> @@ -153,12 +174,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;
> @@ -173,9 +208,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 4/5] max44000: Expose ambient sensor scaling

[Jonathan Cameron]

On 07/04/16 17:21, 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.
Hmm.. This usually means we have oversampling going on rather than straight
integration time control - though it could be actually turning on or off
steps in the ADC pipeline...
> 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
Agreed, this is the easiest way to handle this sort of thing.

This looks good to me (few trivial bits I picked up on inline).
> 
> Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
> ---
>  drivers/iio/light/max44000.c | 163 +++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 159 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/light/max44000.c b/drivers/iio/light/max44000.c
> index 1dc10b9..e479c53 100644
> --- a/drivers/iio/light/max44000.c
> +++ b/drivers/iio/light/max44000.c
> @@ -53,6 +53,12 @@
>  #define MAX44000_CFG_MODE_PRX		0x14
>  #define MAX44000_CFG_TRIM		0x20
>  
> +/* 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
> @@ -73,6 +79,50 @@ 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
Another multiline comment issue. Please check all the patches..
> + *
> + * 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.
> + *
> + * Hanlding this internally is also required for buffer support because the
handling.
> + * 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,
> @@ -94,15 +144,54 @@ static const struct iio_chan_spec max44000_channels[] = {
>  	},
>  };
>  
> +static inline 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 inline 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 inline 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 inline 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 inline int max44000_read_alsval(struct max44000_data *data)
>  {
>  	u16 regval;
> -	int ret;
> +	int alstim, ret;
>  
>  	regval = 0;
>  	ret = regmap_bulk_read(data->regmap, MAX44000_REG_ALS_DATA_HI, &regval, 2);
>  	if (ret < 0)
>  		return ret;
> +	alstim = ret = max44000_read_alstim(data);
> +	if (ret < 0)
> +		return ret;
>  
>  	regval = be16_to_cpu(regval);
>  
> @@ -117,7 +206,7 @@ static inline int max44000_read_alsval(struct max44000_data *data)
>  	if (regval & MAX44000_ALSDATA_OVERFLOW)
>  		return 0x3FFF;
>  
> -	return regval;
> +	return regval << MAX44000_ALSTIM_SHIFT(alstim);
>  }
>  
>  static inline int max44000_write_led_current_raw(struct max44000_data *data, int val)
> @@ -150,6 +239,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;
>  
> @@ -195,14 +285,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;
>  	}
> @@ -220,15 +330,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 5/5] max44000: Initial triggered buffer support

[Jonathan Cameron]

On 07/04/16 17:21, Crestez Dan Leonard wrote:
> Signed-off-by: Crestez Dan Leonard <leonard.crestez@intel.com>
> ---
>  drivers/iio/light/max44000.c | 63 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
> 
> diff --git a/drivers/iio/light/max44000.c b/drivers/iio/light/max44000.c
> index e479c53..7b1f8bc 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"
> @@ -123,24 +126,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	= 8,
> +		}
>  	},
> +	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,
>  	},
>  };
>  
> @@ -456,6 +476,42 @@ static int max44000_force_write_defaults(struct max44000_data *data)
>  	return 0;
>  }
>  
> +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[indio_dev->scan_bytes / 2];
Autobuilder is moaning about this + you shouldn't directly access scan_bytes
at all from a driver.  Here just make sure it's big enough to take anything that might
show up (which isn't terribly big ;)
> +	u8 *pos = (u8 *)buf;
> +	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;
> +		*((u16 *)pos) = ret;
> +		pos += 2;
> +	}
> +	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;
> +		*pos = 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)
>  {
> @@ -513,6 +569,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);
>  }
>  
> @@ -521,6 +583,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;
>  }
>  
> 

Re: [PATCH 1/5] max44000: Initial commit

[Crestez Dan Leonard]

On 04/10/2016 04:12 PM, Jonathan Cameron wrote:
> On 07/04/16 20:48, Peter Meerwald-Stadler wrote:
>>
>>> This just adds support for reporting illuminance with default settings.
>>>
>>> All default registers are written on probe because the device otherwise
>>> lacks a reset function.
>>
>> comments below
> Mostly fine, but a few corners need cleaning up.
>
> Also, I'm not keep on the brute force write everything.  The driver should
> cope with any values in those registers and deal with refreshing the
> cache etc so that it can do so.  Writing a bunch of defaults is rather a
> brittle approach.

But if the hardware is not in the expected state the driver won't report 
correct values, at least not without reading scaling factors.

It's not clear what you mean by brittle?

>>> +struct max44000_data {
>>> +	struct mutex lock;
>>> +	struct i2c_client *client;
> This client pointer isn't used outside probe and remove where it is easily
> available anyway.  Hence don't keep a copy in here.

Ok, will remove

>>> +static bool max44000_readable_reg(struct device *dev, unsigned int reg)
>>> +{
>>> +	return (1 << reg) & MAX44000_REGMASK_READABLE;
> See above.  This is a really nasty and hard to review way of doing this.
> switch (reg) {
>         REG1:
>         REG2:
>         REG3:
> 	  return true;
>         default:
>            return false;
>
> may be more code, but it's easy to tell if it is right.

Won't a switch result in larger executable code? Would it be acceptable 
to just expand the REGMASK_* into a large or-ing of (1 << 
MAX44000_REG_*)? Then it would be clear in the source what's going on 
but binary will be the same.

>>> +static const struct reg_default max44000_reg_defaults[] = {
>>> +	{ MAX44000_REG_CFG_MAIN,	0x24 },
>>> +	/* Upper 4 bits are not documented but start as 1 on powerup
> Multiline comment syntax please.

Ok, I will fix this in all the patches.

>>> +	.use_single_rw	= 1,
>>> +	.cache_type	= REGCACHE_FLAT,
> This always seems like a good idea, but tends to cause issues.
> FLAT is really only meant for very high performance devices, you
> are probably better with something else here.  If you are doing this
> deliberately to make the below writes actually occur, then please
> add a comment here.

I used REGCACHE_FLAT because my device has a very small number of 
registers and I assume it uses less memory. Honestly it would make sense 
for regmap to include a REGCACHE_AUTO cache_type and pick the cache 
implementation automatically based on number of registers.

>>> +static int max44000_force_write_defaults(struct max44000_data *data)
>>> +{
>>> +	int i, ret;
>>> +
>>> +	for (i = 0; i < ARRAY_SIZE(max44000_reg_defaults); ++i) {
>>> +		ret = regmap_write(data->regmap,
>>> +				   max44000_reg_defaults[i].reg,
>>> +				   max44000_reg_defaults[i].def);
> Silly question, but if the cached value matches the values you are trying
> to write here will this work?

Yes. It would not work otherwise since the regmap cache is explicitly 
initialized with my listed defaults.

As far as I can tell regmap_write will always write to the hardware.

> There is a regcache_mark_dirty call that will ensure all registers in the
> cache are read..

The regcache_mark_dirty function is used to notify the cache that the 
device has been reset to the known default values. I attempted to do 
something like:

	regcache_mark_dirty()
	regcache_sync()

This doesn't work because this explicitly avoid overwriting known defaults.

> If you then need any particular values they should be explicitly written
> on the assumption you have no idea what the state is.  Brute force writing
> all the defaults is nasty and doesn't give any information as to what
> is happening.

If the device had a reset command I should have used that, right? What 
is happening is that I am implementing a reset command in software.

I can skip writing values like REG_TRIM_* which are used for calibration 
and are not exposed by the driver. This would allow these values to be 
configured externally using something like i2cset at boot time. Is that 
what you mean?

I could also skip initializing scaling parameters but then stuff like 
in_illuminance_scale would persist after rmmod/insmod. This seems 
undesirable.

--
Regards,
Leonard

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

[Crestez Dan Leonard]

On 04/07/2016 10:59 PM, Peter Meerwald-Stadler wrote:
>>   static int max44000_probe(struct i2c_client *client,
>>   			  const struct i2c_device_id *id)
>>   {
>> @@ -513,6 +569,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);
>
> no devm_ possible anymore :-)

It's not clear to me why explicit calls to iio_triggered_buffer_cleanup 
are done in every driver. Wouldn't it be possible for iio_dev_release to 
call iio_triggered_buffer_cleanup? That would require 
triggered_buffer_cleanup to explictly NULL the fields it deallocates so 
that duplicate cleanups don't crash.

--
Regards,
Leonard

Re: [PATCH 1/5] max44000: Initial commit

[Jonathan Cameron]

On 11/04/16 16:08, Crestez Dan Leonard wrote:
> On 04/10/2016 04:12 PM, Jonathan Cameron wrote:
>> On 07/04/16 20:48, Peter Meerwald-Stadler wrote:
>>>
>>>> This just adds support for reporting illuminance with default settings.
>>>>
>>>> All default registers are written on probe because the device otherwise
>>>> lacks a reset function.
>>>
>>> comments below
>> Mostly fine, but a few corners need cleaning up.
>>
>> Also, I'm not keep on the brute force write everything.  The driver should
>> cope with any values in those registers and deal with refreshing the
>> cache etc so that it can do so.  Writing a bunch of defaults is rather a
>> brittle approach.
> 
> But if the hardware is not in the expected state the driver won't
> report correct values, at least not without reading scaling factors.
Exactly, I'd expect the driver to be reading those scaling factors.

If the device comes up in an entirely random state on power up then
writing the whole register set is fair enough.
> It's not clear what you mean by brittle?
Liable to miss some set of circumstances due to an assumption that
you know what the value of every register is.  It's not a problem now
as you will have gained a good understanding of the device writing
the whole driver - it might result in subtle accidental breakage
if someone tries to make a small change in the future though.

This point isn't that important though as the device isn't all that
complicated so such problems should be easy enough to spot...
> 
>>>> +struct max44000_data {
>>>> +    struct mutex lock;
>>>> +    struct i2c_client *client;
>> This client pointer isn't used outside probe and remove where it is easily
>> available anyway.  Hence don't keep a copy in here.
> 
> Ok, will remove
> 
>>>> +static bool max44000_readable_reg(struct device *dev, unsigned int reg)
>>>> +{
>>>> +    return (1 << reg) & MAX44000_REGMASK_READABLE;
>> See above.  This is a really nasty and hard to review way of doing this.
>> switch (reg) {
>>         REG1:
>>         REG2:
>>         REG3:
>>       return true;
>>         default:
>>            return false;
>>
>> may be more code, but it's easy to tell if it is right.
> 
> Won't a switch result in larger executable code?
Possibly depending on how clever the compiler is feeling.  Not much
more code though I would expect. It's all well described so the
compiler ought to do a good job of optimizing it.
> Would it be
> acceptable to just expand the REGMASK_* into a large or-ing of (1 <<
> MAX44000_REG_*)? Then it would be clear in the source what's going on
> but binary will be the same.
Would be interesting to see but I doubt the optimized code will be that
different, and the switch is pretty much the 'standard' way of handling
these long register lists cleanly.

Often it comes down to doing things the way people expect them to
be done as that makes review easier for a tiny possible cost in
run time.
> 
>>>> +static const struct reg_default max44000_reg_defaults[] = {
>>>> +    { MAX44000_REG_CFG_MAIN,    0x24 },
>>>> +    /* Upper 4 bits are not documented but start as 1 on powerup
>> Multiline comment syntax please.
> 
> Ok, I will fix this in all the patches.
> 
>>>> +    .use_single_rw    = 1,
>>>> +    .cache_type    = REGCACHE_FLAT,
>> This always seems like a good idea, but tends to cause issues.
>> FLAT is really only meant for very high performance devices, you
>> are probably better with something else here.  If you are doing this
>> deliberately to make the below writes actually occur, then please
>> add a comment here.
> 
> I used REGCACHE_FLAT because my device has a very small number of
> registers and I assume it uses less memory. Honestly it would make
> sense for regmap to include a REGCACHE_AUTO cache_type and pick the
> cache implementation automatically based on number of registers.
I've fallen for that one in the past as well.  AUTO would indeed
be good if it was easy to do.

>>>> +static int max44000_force_write_defaults(struct max44000_data *data)
>>>> +{
>>>> +    int i, ret;
>>>> +
>>>> +    for (i = 0; i < ARRAY_SIZE(max44000_reg_defaults); ++i) {
>>>> +        ret = regmap_write(data->regmap,
>>>> +                   max44000_reg_defaults[i].reg,
>>>> +                   max44000_reg_defaults[i].def);
>> Silly question, but if the cached value matches the values you are trying
>> to write here will this work?
> 
> Yes. It would not work otherwise since the regmap cache is explicitly
> initialized with my listed defaults.
> As far as I can tell regmap_write will always write to the hardware.
Interesting and counter intuitive if true...
Taking the rbtree cache If it's not volatile it looks to try
writing in regcache which if it gets a match on the value being in the tree
calls regcache_set_val which checks the cache and will fall through having
written nothing if it thinks it has a match.

Taking FLAT - yeah it doesn't check the cache at all on a write - merely
blindly updates it.

I've cc'd Mark Brown in case he has any comments on this.
> 
>> There is a regcache_mark_dirty call that will ensure all registers in the
>> cache are read..
> 
> The regcache_mark_dirty function is used to notify the cache that the
> device has been reset to the known default values. I attempted to do
> something like:
>     regcache_mark_dirty()
>     regcache_sync()
> 
> This doesn't work because this explicitly avoid overwriting known defaults.
Hmm. I've clearly misunderstood this then.  It has no mention of expecting the
hardware to be in any particular state.
> 
>> If you then need any particular values they should be explicitly written
>> on the assumption you have no idea what the state is.  Brute force writing
>> all the defaults is nasty and doesn't give any information as to what
>> is happening.
> 
> If the device had a reset command I should have used that, right?
> What is happening is that I am implementing a reset command in
> software.
Not necessarily.  Lots of drivers don't - but instead have their interfaces
reflect their current state on startup.  Reset's are often there to get
the internal state of the device cleaned up if it is in an unknowable state
rather than to get the defaults to any particular state.  They are always
read from the hardware or a known good cache when queried from userspace
anyway.
> 
> I can skip writing values like REG_TRIM_* which are used for
> calibration and are not exposed by the driver. This would allow these
> values to be configured externally using something like i2cset at
> boot time. Is that what you mean?
> 
> I could also skip initializing scaling parameters but then stuff like
> in_illuminance_scale would persist after rmmod/insmod. This seems
> undesirable.
Why?  Any userspace should be setting these to the values it wants
not relying on what it 'thinks' the defaults are.

I'm not that fussed about all this, it just seems clunky to need to do
such a wholesale write of registers so if the regmap core can be made
to do that it would definitely be preferable.  You can definitely force
it as a patch to the default set but that seems ugly too.

Jonathan

> -- 
> Regards,
> Leonard

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

[Jonathan Cameron]

On 11/04/16 17:11, Crestez Dan Leonard wrote:
> On 04/07/2016 10:59 PM, Peter Meerwald-Stadler wrote:
>>>   static int max44000_probe(struct i2c_client *client,
>>>                 const struct i2c_device_id *id)
>>>   {
>>> @@ -513,6 +569,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);
>>
>> no devm_ possible anymore :-)
> 
> It's not clear to me why explicit calls to
> iio_triggered_buffer_cleanup are done in every driver. Wouldn't it be
> possible for iio_dev_release to call iio_triggered_buffer_cleanup?
> That would require triggered_buffer_cleanup to explictly NULL the
> fields it deallocates so that duplicate cleanups don't crash.
Because not all devices are using the triggered buffer setup function
thus they can't use the cleanup one.  There are different buffers for
starters and the core has no way to identify them (very deliberately
as it should never care what they are).

There is also the general obvious correctness of code question.
If you do one thing in probe, it should be reverse in remove...

We could in theory do a devm version of the iio_triggered_buffer_cleanup
function though as then the tear down would occur in the write order and
as it is a devm_ call in the probe we'd know how the unwind was being
done...


> -- 
> Regards,
> Leonard

Re: [PATCH 1/5] max44000: Initial commit

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 3024 bytes --]

On Sun, Apr 17, 2016 at 09:36:10AM +0100, Jonathan Cameron wrote:
> On 11/04/16 16:08, Crestez Dan Leonard wrote:

Please leave blank lines between paragraphs, it makes things much easier
to read.

> > Would it be
> > acceptable to just expand the REGMASK_* into a large or-ing of (1 <<
> > MAX44000_REG_*)? Then it would be clear in the source what's going on
> > but binary will be the same.

> Would be interesting to see but I doubt the optimized code will be that
> different, and the switch is pretty much the 'standard' way of handling
> these long register lists cleanly.

> Often it comes down to doing things the way people expect them to
> be done as that makes review easier for a tiny possible cost in
> run time.

You can also specify ranges of registers if the map mostly has large
blocks of contiguous registers, a switch statement tends to be easier
and is probably more efficient for most register maps.

> >>>> +    .use_single_rw    = 1,
> >>>> +    .cache_type    = REGCACHE_FLAT,

> >> This always seems like a good idea, but tends to cause issues.
> >> FLAT is really only meant for very high performance devices, you
> >> are probably better with something else here.  If you are doing this
> >> deliberately to make the below writes actually occur, then please
> >> add a comment here.

> > I used REGCACHE_FLAT because my device has a very small number of
> > registers and I assume it uses less memory. Honestly it would make
> > sense for regmap to include a REGCACHE_AUTO cache_type and pick the
> > cache implementation automatically based on number of registers.

> I've fallen for that one in the past as well.  AUTO would indeed
> be good if it was easy to do.

It's extremely easy to do.  Unless you've got a good reason to do
anything else you should always be using an rbtree.  The core would
never select anything else.

> > Yes. It would not work otherwise since the regmap cache is explicitly
> > initialized with my listed defaults.
> > As far as I can tell regmap_write will always write to the hardware.

> Interesting and counter intuitive if true...

No, if the driver asked to write then we write.  If the driver wants to
do a read/modify/write cycle it should use regmap_update_bits().

> > If the device had a reset command I should have used that, right?
> > What is happening is that I am implementing a reset command in
> > software.

> Not necessarily.  Lots of drivers don't - but instead have their interfaces
> reflect their current state on startup.  Reset's are often there to get
> the internal state of the device cleaned up if it is in an unknowable state
> rather than to get the defaults to any particular state.  They are always
> read from the hardware or a known good cache when queried from userspace
> anyway.

That's not entirely it.  Doing a reset is often faster than rewriting
the entire register map and is more robust against undocumented
registers or things the driver didn't think about which means that
the behaviour is going to be more consistent.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/5] max44000: Initial commit

[Lars-Peter Clausen]

[-- Attachment #1: Type: text/plain, Size: 1971 bytes --]

On 04/18/2016 12:32 PM, Mark Brown wrote:
[...]
>>>> This always seems like a good idea, but tends to cause issues.
>>>> FLAT is really only meant for very high performance devices, you
>>>> are probably better with something else here.  If you are doing this
>>>> deliberately to make the below writes actually occur, then please
>>>> add a comment here.
> 
>>> I used REGCACHE_FLAT because my device has a very small number of
>>> registers and I assume it uses less memory. Honestly it would make
>>> sense for regmap to include a REGCACHE_AUTO cache_type and pick the
>>> cache implementation automatically based on number of registers.
> 
>> I've fallen for that one in the past as well.  AUTO would indeed
>> be good if it was easy to do.
> 
> It's extremely easy to do.  Unless you've got a good reason to do
> anything else you should always be using an rbtree.  The core would
> never select anything else.

Just to add some technical background, maybe that helps to clear things up.
The rbtree does not have a one node for each register, it has one node for
each continuous register region. You can think of the rbtree regmap as a
tree with a flat cache at each node. If there is only one continuous region
there will only be one node and the behavior is very similar to the flat cache.

The memory overhead is the size of single node, which is usually negligible.

For register reads and writes there is a slight overhead of looking up the
node. But since the rbtree caches the node that was looked up last the
overhead is just checking if the current node is still the correct one
(which it will be since there is only one node). This check is about 4-5 hw
instructions which is completely negligible to the time it takes to execute
the SPI or I2C transfer. The only place where flat makes sense is where the
hardware register access itself only takes a few CPU cycles and where the
overhead of the lookup is noticeable.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/5] max44000: Initial commit

[Crestez Dan Leonard]

On 04/18/2016 01:32 PM, Mark Brown wrote:
> On Sun, Apr 17, 2016 at 09:36:10AM +0100, Jonathan Cameron wrote:
>> On 11/04/16 16:08, Crestez Dan Leonard wrote:
>>> I used REGCACHE_FLAT because my device has a very small number of
>>> registers and I assume it uses less memory. Honestly it would make
>>> sense for regmap to include a REGCACHE_AUTO cache_type and pick the
>>> cache implementation automatically based on number of registers.
> 
>> I've fallen for that one in the past as well.  AUTO would indeed
>> be good if it was easy to do.
> 
> It's extremely easy to do.  Unless you've got a good reason to do
> anything else you should always be using an rbtree.  The core would
> never select anything else.

Ok, I will remember this.

>>> Yes. It would not work otherwise since the regmap cache is explicitly
>>> initialized with my listed defaults.
>>> As far as I can tell regmap_write will always write to the hardware.
> 
>> Interesting and counter intuitive if true...
> 
> No, if the driver asked to write then we write.  If the driver wants to
> do a read/modify/write cycle it should use regmap_update_bits().

As a further clarification: regmap_write will write to hardware even if
the cache is known to be up-to-date and no matter the regcache_type. Did
I understand this correctly?

I'm basing this on reading the code, it seems to me that map->reg_write
is only avoided on error paths or if map->cache_only is set to true.

This always-write guarantee is not obvious and if it's OK for drivers to
rely on it perhaps it should be explicitly documented on regmap_write.
Otherwise for my device I would need some way to mention that the device
starts in an undefined state, not what is specified in reg_defaults.

For simplicity I will drop regmap_config.reg_defaults completely and
just setup the few parameters I need explicitly. This will be in v3.

--
Regards,
Leonard

Re: [PATCH 1/5] max44000: Initial commit

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 698 bytes --]

On Mon, Apr 18, 2016 at 03:15:54PM +0300, Crestez Dan Leonard wrote:

> As a further clarification: regmap_write will write to hardware even if
> the cache is known to be up-to-date and no matter the regcache_type. Did
> I understand this correctly?

> I'm basing this on reading the code, it seems to me that map->reg_write
> is only avoided on error paths or if map->cache_only is set to true.

> This always-write guarantee is not obvious and if it's OK for drivers to
> rely on it perhaps it should be explicitly documented on regmap_write.

Yes.  I have to say that you are the first person I've encountered who
has been confused by this, I'm not sure why you'd expect writes to be
discarded.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/5] max44000: Initial commit

[Jonathan Cameron]

On 18/04/16 13:34, Mark Brown wrote:
> On Mon, Apr 18, 2016 at 03:15:54PM +0300, Crestez Dan Leonard wrote:
> 
>> As a further clarification: regmap_write will write to hardware even if
>> the cache is known to be up-to-date and no matter the regcache_type. Did
>> I understand this correctly?
> 
>> I'm basing this on reading the code, it seems to me that map->reg_write
>> is only avoided on error paths or if map->cache_only is set to true.
> 
>> This always-write guarantee is not obvious and if it's OK for drivers to
>> rely on it perhaps it should be explicitly documented on regmap_write.
> 
> Yes.  I have to say that you are the first person I've encountered who
> has been confused by this, I'm not sure why you'd expect writes to be
> discarded.
> 
It confused me too :)  To my mind a classic cache optimization would be
to not write to the hardware if the value is already known to be as
desired.

Still, I guess it would add another check to identify which
registers you really wanted to hammer whatever vs which can be assumed
not to read the write is not worth the effort for this case that 
inherently won't be hit that often.

Jonathan 

Re: [PATCH 1/5] max44000: Initial commit

[Jonathan Cameron]

On 18/04/16 11:32, Mark Brown wrote:
> On Sun, Apr 17, 2016 at 09:36:10AM +0100, Jonathan Cameron wrote:
>> On 11/04/16 16:08, Crestez Dan Leonard wrote:
> 
> Please leave blank lines between paragraphs, it makes things much easier
> to read.
> 
>>> Would it be
>>> acceptable to just expand the REGMASK_* into a large or-ing of (1 <<
>>> MAX44000_REG_*)? Then it would be clear in the source what's going on
>>> but binary will be the same.
> 
>> Would be interesting to see but I doubt the optimized code will be that
>> different, and the switch is pretty much the 'standard' way of handling
>> these long register lists cleanly.
> 
>> Often it comes down to doing things the way people expect them to
>> be done as that makes review easier for a tiny possible cost in
>> run time.
> 
> You can also specify ranges of registers if the map mostly has large
> blocks of contiguous registers, a switch statement tends to be easier
> and is probably more efficient for most register maps.
> 
>>>>>> +    .use_single_rw    = 1,
>>>>>> +    .cache_type    = REGCACHE_FLAT,
> 
>>>> This always seems like a good idea, but tends to cause issues.
>>>> FLAT is really only meant for very high performance devices, you
>>>> are probably better with something else here.  If you are doing this
>>>> deliberately to make the below writes actually occur, then please
>>>> add a comment here.
> 
>>> I used REGCACHE_FLAT because my device has a very small number of
>>> registers and I assume it uses less memory. Honestly it would make
>>> sense for regmap to include a REGCACHE_AUTO cache_type and pick the
>>> cache implementation automatically based on number of registers.
> 
>> I've fallen for that one in the past as well.  AUTO would indeed
>> be good if it was easy to do.
> 
> It's extremely easy to do.  Unless you've got a good reason to do
> anything else you should always be using an rbtree.  The core would
> never select anything else.
> 
>>> Yes. It would not work otherwise since the regmap cache is explicitly
>>> initialized with my listed defaults.
>>> As far as I can tell regmap_write will always write to the hardware.
> 
>> Interesting and counter intuitive if true...
> 
> No, if the driver asked to write then we write.  If the driver wants to
> do a read/modify/write cycle it should use regmap_update_bits().
> 
>>> If the device had a reset command I should have used that, right?
>>> What is happening is that I am implementing a reset command in
>>> software.
> 
>> Not necessarily.  Lots of drivers don't - but instead have their interfaces
>> reflect their current state on startup.  Reset's are often there to get
>> the internal state of the device cleaned up if it is in an unknowable state
>> rather than to get the defaults to any particular state.  They are always
>> read from the hardware or a known good cache when queried from userspace
>> anyway.
> 
> That's not entirely it.  Doing a reset is often faster than rewriting
> the entire register map and is more robust against undocumented
> registers or things the driver didn't think about which means that
> the behaviour is going to be more consistent.
Hmm. Fair enough on the undocumented register argument...
> 

Re: [PATCH 1/5] max44000: Initial commit

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 859 bytes --]

On Mon, Apr 18, 2016 at 08:36:19PM +0100, Jonathan Cameron wrote:
> On 18/04/16 13:34, Mark Brown wrote:

> > Yes.  I have to say that you are the first person I've encountered who
> > has been confused by this, I'm not sure why you'd expect writes to be
> > discarded.

> It confused me too :)  To my mind a classic cache optimization would be
> to not write to the hardware if the value is already known to be as
> desired.

> Still, I guess it would add another check to identify which
> registers you really wanted to hammer whatever vs which can be assumed
> not to read the write is not worth the effort for this case that 
> inherently won't be hit that often.

We'd also have to add another API for cases where someone explicitly
wants to write the same thing to the hardware, you get things like
latched "do it" bits in registers.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]