Support for max44000 Ambient and Infrared Proximity Sensor

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 <[email protected]>

---
 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

drivers/iio/light/Kconfig +11 -0

index cfd3df8..42c15c1 100644
@@ -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

drivers/iio/light/Makefile +0 -0

index b2c3105..14b2f36 100644

drivers/iio/light/max44000.c new file +57 -0

new file mode 100644
index 0000000..7c12153
@@ -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

drivers/iio/light/max44000.c new file +164 -0

+	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;

drivers/iio/light/max44000.c new file +48 -0

+}
+
+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 <[email protected]>");
+MODULE_DESCRIPTION("MAX44000 Ambient and Infrared Proximity Sensor");
+MODULE_LICENSE("GPL v2");

Quoted 5 lines

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

Quoted 34 lines

Signed-off-by Crestez Dan Leonard <[email protected]>

---
 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

drivers/iio/light/Kconfig +11 -0

index cfd3df8..42c15c1 100644
@@ -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

drivers/iio/light/Makefile +0 -0

index b2c3105..14b2f36 100644

drivers/iio/light/Makefile obj-$(CONFIG_TCS3472) += tcs3472.o · line 33

Quoted 4 lines

drivers/iio/light/Makefile +1 -0

@@ -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

Quoted 46 lines

drivers/iio/light/max44000.c new file +40 -0

new file mode 100644
index 0000000..7c12153
@@ -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

drivers/iio/light/max44000.c line 48

Quoted 8 lines

drivers/iio/light/max44000.c new file +8 -0

+#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

drivers/iio/light/max44000.c line 52

Quoted 4 lines

drivers/iio/light/max44000.c new file +4 -0

+
+struct max44000_data {
+	struct mutex lock;
+	struct i2c_client *client;

Quoted 5 lines

drivers/iio/light/max44000.c new file +5 -0

+	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

drivers/iio/light/max44000.c line 67

Quoted 10 lines

drivers/iio/light/max44000.c new file +10 -0

+
+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

drivers/iio/light/max44000.c line 72

Quoted 5 lines

drivers/iio/light/max44000.c new file +5 -0

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

needed?

drivers/iio/light/max44000.c line 73

Quoted 1 line

drivers/iio/light/max44000.c new file +1 -0

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

sizeof(regval)

Quoted 58 lines

drivers/iio/light/max44000.c new file +58 -0

+	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;
+	}
+}
+

drivers/iio/light/max44000.c line 139

Quoted 8 lines

drivers/iio/light/max44000.c new file +8 -0

+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;

Quoted 13 lines

drivers/iio/light/max44000.c new file +13 -0

+}
+
+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)

drivers/iio/light/max44000.c line 160

Quoted 8 lines

drivers/iio/light/max44000.c new file +8 -0

+{
+	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

Quoted 19 lines

drivers/iio/light/max44000.c new file +19 -0

+	 * 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,
+

drivers/iio/light/max44000.c line 187

Quoted 8 lines

drivers/iio/light/max44000.c new file +8 -0

+	.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,

Quoted 5 lines

drivers/iio/light/max44000.c new file +5 -0

+
+	.reg_defaults		= max44000_reg_defaults,
+	.num_reg_defaults	= ARRAY_SIZE(max44000_reg_defaults),
+};
+

drivers/iio/light/max44000.c line 200

Quoted 8 lines

drivers/iio/light/max44000.c new file +8 -0

+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);

Quoted 37 lines

drivers/iio/light/max44000.c new file +37 -0

+		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;

drivers/iio/light/max44000.c line 247

Quoted 10 lines

drivers/iio/light/max44000.c new file +10 -0

+	}
+
+	/* 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

Quoted 49 lines

drivers/iio/light/max44000.c new file +48 -0

+}
+
+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 <[email protected]>");
+MODULE_DESCRIPTION("MAX44000 Ambient and Infrared Proximity Sensor");
+MODULE_LICENSE("GPL v2");

Quoted 6 lines · Peter Meerwald-Stadler

Quoted 5 lines

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.

Quoted 1 line

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.

Quoted 3 lines

drivers/iio/light/max44000.c line 52

Quoted 3 lines

drivers/iio/light/max44000.c new file +3 -0

+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.

Quoted 3 lines

drivers/iio/light/max44000.c line 139

Quoted 3 lines

drivers/iio/light/max44000.c new file +3 -0

+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.

Quoted 3 lines

drivers/iio/light/max44000.c line 160

Quoted 3 lines

drivers/iio/light/max44000.c new file +3 -0

+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.

Quoted 2 lines

drivers/iio/light/max44000.c line 187

Quoted 2 lines

drivers/iio/light/max44000.c new file +2 -0

+	.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.

Quoted 8 lines

drivers/iio/light/max44000.c line 200

Quoted 8 lines

drivers/iio/light/max44000.c new file +8 -0

+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.

Quoted 10 lines · Jonathan Cameron

Quoted 7 lines · Peter Meerwald-Stadler

Quoted 5 lines

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.

Quoted 4 lines

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?

Quoted 6 lines

Quoted 4 lines

drivers/iio/light/max44000.c line 52

Quoted 4 lines

drivers/iio/light/max44000.c new file +3 -0

+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

Quoted 12 lines

Quoted 3 lines

drivers/iio/light/max44000.c line 139

Quoted 3 lines

drivers/iio/light/max44000.c new file +3 -0

+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;

Quoted 1 line

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.

Quoted 5 lines

Quoted 4 lines

drivers/iio/light/max44000.c line 160

Quoted 4 lines

drivers/iio/light/max44000.c new file +3 -0

+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.

Quoted 7 lines

Quoted 2 lines

drivers/iio/light/max44000.c line 187

Quoted 2 lines

drivers/iio/light/max44000.c new file +2 -0

+    .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.

Quoted 10 lines

Quoted 8 lines

drivers/iio/light/max44000.c line 200

Quoted 8 lines

drivers/iio/light/max44000.c new file +8 -0

+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.

Quoted 3 lines

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.

Quoted 5 lines

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.

Signature · 2 lines

Regards,
Leonard

On 11/04/16 16:08, Crestez Dan Leonard wrote:

Quoted 4 lines

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.

Quoted 8 lines

Quoted 8 lines

Quoted 2 lines

drivers/iio/light/max44000.c line 187

Quoted 2 lines

drivers/iio/light/max44000.c new file +2 -0

+    .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.

Quoted 4 lines

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.

Quoted 3 lines

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...

Quoted 3 lines

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.

Quoted 10 lines

Quoted 10 lines

Quoted 5 lines

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.

Quoted 2 lines

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.

Quoted 5 lines · Jonathan Cameron

Quoted 4 lines · Crestez Dan Leonard

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.

Quoted 2 lines

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.

Quoted 5 lines

Quoted 3 lines

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.

Quoted 6 lines · Crestez Dan Leonard

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.

Quoted 2 lines

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.

Quoted 3 lines · Mark Brown

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.

Quoted 1 line · Jonathan Cameron

On 11/04/16 16:08, Crestez Dan Leonard wrote:

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

Quoted 5 lines

Quoted 4 lines

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.

Quoted 7 lines

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.

Quoted 9 lines

Quoted 8 lines

Quoted 8 lines

Quoted 2 lines

drivers/iio/light/max44000.c line 187

Quoted 2 lines

drivers/iio/light/max44000.c new file +2 -0

+    .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.

Quoted 7 lines

Quoted 4 lines

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.

Quoted 5 lines

Quoted 3 lines

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().

Quoted 10 lines

Quoted 3 lines

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.

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 <[email protected]>

---
 drivers/iio/light/max44000.c | 61 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

drivers/iio/light/max44000.c +44 -0

index 7c12153..10a0fe8 100644
@@ -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;
 		}

drivers/iio/light/max44000.c +16 -0

+	 * 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);
 }
 

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 <[email protected]>

---
 drivers/iio/light/max44000.c | 53 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

drivers/iio/light/max44000.c +53 -0

index 10a0fe8..1dc10b9 100644
@@ -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)

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 <[email protected]>

---
 drivers/iio/light/max44000.c | 163 +++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 159 insertions(+), 4 deletions(-)

drivers/iio/light/max44000.c +6 -0

index 1dc10b9..e479c53 100644
@@ -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

drivers/iio/light/max44000.c +140 -4

+ * 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)

Signed-off-by Crestez Dan Leonard <[email protected]>

---
 drivers/iio/light/max44000.c | 63 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

drivers/iio/light/max44000.c +12 -0

index e479c53..7b1f8bc 100644
@@ -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,
+		}

drivers/iio/light/max44000.c +3 -0

+		}
 	},
+	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,
 	},
 };
 

drivers/iio/light/max44000.c +9 -0

+	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)
 {

drivers/iio/light/max44000.c +1 -0

 }
 

@@ -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;
 }
 

Quoted 7 lines

static int max44000_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{

drivers/iio/light/max44000.c +0 -0

@@ -513,6 +569,12 @@ static int max44000_probe(struct i2c_client *client,
  		return ret;
  	}
 

drivers/iio/light/max44000.c static int max44000_probe(struct i2c_client *client, · line 578

Quoted 7 lines

drivers/iio/light/max44000.c +6 -0

+	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 :-)

Quoted 7 lines · Peter Meerwald-Stadler

Quoted 7 lines

static int max44000_probe(struct i2c_client *client,
const struct i2c_device_id *id)
{

drivers/iio/light/max44000.c +0 -0

@@ -513,6 +569,12 @@ static int max44000_probe(struct i2c_client *client,
          return ret;
      }
 

Quoted 9 lines

drivers/iio/light/max44000.c static int max44000_probe(struct i2c_client *client, · line 578

Quoted 7 lines

drivers/iio/light/max44000.c +6 -0

+    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.

Signature · 2 lines

Regards,
Leonard

Signed-off-by Crestez Dan Leonard <[email protected]>

---
 drivers/iio/light/max44000.c | 63 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

drivers/iio/light/max44000.c +20 -0

index e479c53..7b1f8bc 100644
@@ -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,
 	},
 };
 

drivers/iio/light/max44000.c +37 -0

+	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;
 }