The Allegro MicroSystems ALS31300 is a 3-D Linear Hall Effect Sensor
mainly used for 3D head-on motion sensing applications.

The device is configured over I2C, and as part of the Sensor
data the temperature core is also provided.

While the device provides an IRQ gpio, it depends on a configuration
programmed into the internal EEPROM, thus only the default mode
is supported and buffered input via trigger is also supported
to allow streaming values with the same sensing timestamp.

The device can be configured with different sensitivities in factory,
but the sensitivity value used to calculate value into the Gauss
unit is not available from registers, thus the sensitivity is
provided by the compatible/device-id string which is based
on the part number as described in the datasheet page 2.

Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
---
 drivers/iio/magnetometer/Kconfig    |  13 +
 drivers/iio/magnetometer/Makefile   |   1 +
 drivers/iio/magnetometer/als31300.c | 459 ++++++++++++++++++++++++++++++++++++
 3 files changed, 473 insertions(+)

diff --git a/drivers/iio/magnetometer/Kconfig b/drivers/iio/magnetometer/Kconfig
index 8eb718f5e50f..e7adc11049d6 100644
--- a/drivers/iio/magnetometer/Kconfig
+++ b/drivers/iio/magnetometer/Kconfig
@@ -52,6 +52,19 @@ config AK09911
 	help
 	  Deprecated: AK09911 is now supported by AK8975 driver.
 
+config ALS31300
+	tristate "Allegro MicroSystems ALS31300 3-D Linear Hall Effect Sensor"
+	depends on I2C
+	select REGMAP_I2C
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
+	help
+	  Say yes here to build support for the Allegro MicroSystems
+	  ALS31300 Hall Effect Sensor through its I2C interface.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called als31300.
+
 config BMC150_MAGN
 	tristate
 	select IIO_BUFFER
diff --git a/drivers/iio/magnetometer/Makefile b/drivers/iio/magnetometer/Makefile
index ec5c46fbf999..3e4c2ecd9adf 100644
--- a/drivers/iio/magnetometer/Makefile
+++ b/drivers/iio/magnetometer/Makefile
@@ -7,6 +7,7 @@
 obj-$(CONFIG_AF8133J)	+= af8133j.o
 obj-$(CONFIG_AK8974)	+= ak8974.o
 obj-$(CONFIG_AK8975)	+= ak8975.o
+obj-$(CONFIG_ALS31300)	+= als31300.o
 obj-$(CONFIG_BMC150_MAGN) += bmc150_magn.o
 obj-$(CONFIG_BMC150_MAGN_I2C) += bmc150_magn_i2c.o
 obj-$(CONFIG_BMC150_MAGN_SPI) += bmc150_magn_spi.o
diff --git a/drivers/iio/magnetometer/als31300.c b/drivers/iio/magnetometer/als31300.c
new file mode 100644
index 000000000000..123e6a63b516
--- /dev/null
+++ b/drivers/iio/magnetometer/als31300.c
@@ -0,0 +1,459 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Driver for the Allegro MicroSystems ALS31300 3-D Linear Hall Effect Sensor
+ *
+ * Copyright (c) 2024 Linaro Limited
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/delay.h>
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/regmap.h>
+#include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
+
+#include <linux/iio/buffer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+
+/*
+ * The Allegro MicroSystems ALS31300 has an EEPROM space to configure how
+ * the device works and how the interrupt line behaves,
+ * we only support the default setup with external trigger
+ *
+ * Since by default the interrupt line is disable, we don't
+ * support GPIO interrupt events for now.
+ *
+ * It should be possible to adapt the driver to the current
+ * device setup, but we leave it as a future exercise.
+ */
+
+#define ALS31300_EEPROM_CONFIG		0x02
+#define ALS31300_EEPROM_INTERRUPT	0x03
+#define ALS31300_EEPROM_CUSTOMER_1	0x0d
+#define ALS31300_EEPROM_CUSTOMER_2	0x0e
+#define ALS31300_EEPROM_CUSTOMER_3	0x0f
+#define ALS31300_VOLATILE_MODE		0x27
+#define ALS31300_VOLATILE_MODE_LPDCM		GENMASK(6, 4)
+#define ALS31300_VOLATILE_MODE_SLEEP		GENMASK(1, 0)
+#define ALS31300_VOLATILE_MSB		0x28
+#define ALS31300_VOLATILE_MSB_TEMPERATURE	GENMASK(5, 0)
+#define ALS31300_VOLATILE_MSB_INTERRUPT		BIT(6)
+#define ALS31300_VOLATILE_MSB_NEW_DATA		BIT(7)
+#define ALS31300_VOLATILE_MSB_Z_AXIS		GENMASK(15, 8)
+#define ALS31300_VOLATILE_MSB_Y_AXIS		GENMASK(23, 16)
+#define ALS31300_VOLATILE_MSB_X_AXIS		GENMASK(31, 24)
+#define ALS31300_VOLATILE_LSB		0x29
+#define ALS31300_VOLATILE_LSB_TEMPERATURE	GENMASK(5, 0)
+#define ALS31300_VOLATILE_LSB_HALL_STATUS	GENMASK(7, 7)
+#define ALS31300_VOLATILE_LSB_Z_AXIS		GENMASK(11, 8)
+#define ALS31300_VOLATILE_LSB_Y_AXIS		GENMASK(15, 12)
+#define ALS31300_VOLATILE_LSB_X_AXIS		GENMASK(19, 16)
+#define ALS31300_VOLATILE_LSB_INTERRUPT_WRITE	BIT(20)
+#define ALS31300_CUSTOMER_ACCESS	0x35
+
+#define ALS31300_LPDCM_INACTIVE_0_5_MS		0
+#define ALS31300_LPDCM_INACTIVE_1_0_MS		1
+#define ALS31300_LPDCM_INACTIVE_5_0_MS		2
+#define ALS31300_LPDCM_INACTIVE_10_0_MS		3
+#define ALS31300_LPDCM_INACTIVE_50_0_MS		4
+#define ALS31300_LPDCM_INACTIVE_100_0_MS	5
+#define ALS31300_LPDCM_INACTIVE_500_0_MS	6
+#define ALS31300_LPDCM_INACTIVE_1000_0_MS	7
+
+#define ALS31300_VOLATILE_MODE_ACTIVE_MODE	0
+#define ALS31300_VOLATILE_MODE_SLEEP_MODE	1
+#define ALS31300_VOLATILE_MODE_LPDCM_MODE	2
+
+#define ALS31300_DATA_X_GET(__buf)			\
+		((int)(s8)FIELD_GET(ALS31300_VOLATILE_MSB_X_AXIS, __buf[0]) << 4 | \
+			  FIELD_GET(ALS31300_VOLATILE_LSB_X_AXIS, __buf[1]))
+#define ALS31300_DATA_Y_GET(__buf)			\
+		((int)(s8)FIELD_GET(ALS31300_VOLATILE_MSB_Y_AXIS, __buf[0]) << 4 | \
+			  FIELD_GET(ALS31300_VOLATILE_LSB_Y_AXIS, __buf[1]))
+#define ALS31300_DATA_Z_GET(__buf)			\
+		((int)(s8)FIELD_GET(ALS31300_VOLATILE_MSB_Z_AXIS, __buf[0]) << 4 | \
+			  FIELD_GET(ALS31300_VOLATILE_LSB_Z_AXIS, __buf[1]))
+#define ALS31300_TEMPERATURE_GET(__buf)			\
+		((u32)(u8)FIELD_GET(ALS31300_VOLATILE_MSB_TEMPERATURE, __buf[0]) << 6 | \
+			  FIELD_GET(ALS31300_VOLATILE_LSB_TEMPERATURE, __buf[1]))
+
+enum als31300_channels {
+	TEMPERATURE = 0,
+	AXIS_X,
+	AXIS_Y,
+	AXIS_Z,
+};
+
+struct als31300_data {
+	struct device *dev;
+	/* protects power on/off the device and access HW */
+	struct mutex mutex;
+	unsigned long sensitivity;
+	struct regmap *map;
+	struct {
+		u16 temperature;
+		s16 channels[3];
+		s64 timestamp __aligned(8);
+	} scan;
+};
+
+/* The whole measure is split into 2x32bit registers, we need to read them both at once */
+static int als31300_get_measure(struct als31300_data *data, s16 *t, s16 *x,
+				s16 *y, s16 *z)
+{
+	unsigned int count = 0;
+	u32 buf[2];
+	int ret;
+
+	mutex_lock(&data->mutex);
+	ret = pm_runtime_resume_and_get(data->dev);
+	if (ret)
+		goto unlock;
+
+	/* Max update rate it 2KHz, wait up to 1ms */
+	while (count < 50) {
+		/* Read Data */
+		ret = regmap_bulk_read(data->map, ALS31300_VOLATILE_MSB, buf, 2);
+		if (ret) {
+			dev_err(data->dev, "read data failed, error %d\n", ret);
+			goto out;
+		}
+
+		/* Check if data is valid, happens right after getting out of sleep mode */
+		if (FIELD_GET(ALS31300_VOLATILE_MSB_NEW_DATA, buf[0]))
+			break;
+
+		usleep_range(10, 20);
+		++count;
+	}
+
+	if (count >= 50) {
+		ret = -ETIMEDOUT;
+		goto out;
+	}
+
+	*t = ALS31300_TEMPERATURE_GET(buf);
+	*x = ALS31300_DATA_X_GET(buf);
+	*y = ALS31300_DATA_Y_GET(buf);
+	*z = ALS31300_DATA_Z_GET(buf);
+
+out:
+	pm_runtime_mark_last_busy(data->dev);
+	pm_runtime_put_autosuspend(data->dev);
+
+unlock:
+	mutex_unlock(&data->mutex);
+
+	return ret;
+}
+
+static int als31300_read_raw(struct iio_dev *indio_dev,
+			     const struct iio_chan_spec *chan, int *val,
+			     int *val2, long mask)
+{
+	struct als31300_data *data = iio_priv(indio_dev);
+	s16 t, x, y, z;
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_PROCESSED:
+	case IIO_CHAN_INFO_RAW:
+		ret = als31300_get_measure(data, &t, &x, &y, &z);
+		if (ret)
+			return ret;
+		switch (chan->address) {
+		case TEMPERATURE:
+			*val = t;
+			return IIO_VAL_INT;
+		case AXIS_X:
+			*val = x;
+			return IIO_VAL_INT;
+		case AXIS_Y:
+			*val = y;
+			return IIO_VAL_INT;
+		case AXIS_Z:
+			*val = z;
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->type) {
+		case IIO_TEMP:
+			/*
+			 * Fractional part of:
+			 *         302(value - 1708)
+			 * temp = ------------------
+			 *             4096
+			 * to convert temperature in Celcius
+			 */
+			*val = 302;
+			*val2 = 4096;
+			return IIO_VAL_FRACTIONAL;
+		case IIO_MAGN:
+			/*
+			 * Devices are configured in factory
+			 * with different sensitivities:
+			 * - 500 GAUSS <-> 4 LSB/Gauss
+			 * - 1000 GAUSS <-> 2 LSB/Gauss
+			 * - 2000 GAUSS <-> 1 LSB/Gauss
+			 * with translates by a division of the returned
+			 * value to get Gauss value.
+			 * The sensisitivity cannot be read at runtime
+			 * so the value depends on the model compatible
+			 * or device id.
+			 */
+			*val = 1;
+			*val2 = data->sensitivity;
+			return IIO_VAL_FRACTIONAL;
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_OFFSET:
+		switch (chan->type) {
+		case IIO_TEMP:
+			*val = -1708;
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static irqreturn_t als31300_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct als31300_data *data = iio_priv(indio_dev);
+	s16 x, y, z;
+	u16 t;
+	int ret;
+
+	ret = als31300_get_measure(data, &t, &x, &y, &z);
+	if (ret)
+		goto trigger_out;
+
+	data->scan.temperature = t;
+	data->scan.channels[0] = x;
+	data->scan.channels[1] = y;
+	data->scan.channels[2] = z;
+	iio_push_to_buffers_with_timestamp(indio_dev, &data->scan,
+					   iio_get_time_ns(indio_dev));
+
+trigger_out:
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
+#define ALS31300_AXIS_CHANNEL(axis, index)				     \
+	{								     \
+		.type = IIO_MAGN,					     \
+		.modified = 1,						     \
+		.channel2 = IIO_MOD_##axis,				     \
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |		     \
+				      BIT(IIO_CHAN_INFO_SCALE),		     \
+		.address = index,					     \
+		.scan_index = index,					     \
+		.scan_type = {						     \
+			.sign = 's',					     \
+			.realbits = 12,					     \
+			.storagebits = 16,				     \
+			.endianness = IIO_CPU,				     \
+		},							     \
+	}
+
+static const struct iio_chan_spec als31300_channels[] = {
+	{
+		.type = IIO_TEMP,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+			BIT(IIO_CHAN_INFO_SCALE) |
+			BIT(IIO_CHAN_INFO_OFFSET),
+		.address = TEMPERATURE,
+		.scan_index = TEMPERATURE,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_CPU,
+		},
+	},
+	ALS31300_AXIS_CHANNEL(X, AXIS_X),
+	ALS31300_AXIS_CHANNEL(Y, AXIS_Y),
+	ALS31300_AXIS_CHANNEL(Z, AXIS_Z),
+	IIO_CHAN_SOFT_TIMESTAMP(6),
+};
+
+static const struct iio_info als31300_info = {
+	.read_raw = als31300_read_raw,
+};
+
+static int als31300_set_operating_mode(struct als31300_data *data,
+				       unsigned int val)
+{
+	return regmap_update_bits(data->map, ALS31300_VOLATILE_MODE,
+				  ALS31300_VOLATILE_MODE_SLEEP, val);
+}
+
+static void als31300_power_down(void *data)
+{
+	als31300_set_operating_mode(data, ALS31300_VOLATILE_MODE_SLEEP_MODE);
+}
+
+static const struct iio_buffer_setup_ops als31300_setup_ops = {};
+
+static const unsigned long als31300_scan_masks[] = { GENMASK(3, 0), 0 };
+
+static bool als31300_volatile_reg(struct device *dev, unsigned int reg)
+{
+	return reg == ALS31300_VOLATILE_MSB || reg == ALS31300_VOLATILE_LSB;
+}
+
+static const struct regmap_config als31300_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 32,
+	.max_register = ALS31300_CUSTOMER_ACCESS,
+	.volatile_reg = als31300_volatile_reg,
+};
+
+static int als31300_probe(struct i2c_client *i2c)
+{
+	struct device *dev = &i2c->dev;
+	struct als31300_data *data;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	data = iio_priv(indio_dev);
+	data->dev = dev;
+	i2c_set_clientdata(i2c, indio_dev);
+
+	mutex_init(&data->mutex);
+
+	data->sensitivity = (unsigned long)of_device_get_match_data(dev);
+
+	data->map = devm_regmap_init_i2c(i2c, &als31300_regmap_config);
+	if (IS_ERR(data->map))
+		return dev_err_probe(dev, PTR_ERR(data->map),
+				     "failed to allocate register map\n");
+
+	ret = devm_regulator_get_enable(dev, "vcc");
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to enable regulator\n");
+
+	ret = als31300_set_operating_mode(data, ALS31300_VOLATILE_MODE_ACTIVE_MODE);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to power on device\n");
+
+	ret = devm_add_action_or_reset(dev, als31300_power_down, data);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to add powerdown action\n");
+
+	indio_dev->info = &als31300_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->name = i2c->name;
+	indio_dev->channels = als31300_channels;
+	indio_dev->num_channels = ARRAY_SIZE(als31300_channels);
+	indio_dev->available_scan_masks = als31300_scan_masks;
+
+	ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
+					      iio_pollfunc_store_time,
+					      als31300_trigger_handler,
+					      &als31300_setup_ops);
+	if (ret < 0) {
+		dev_err(dev, "iio triggered buffer setup failed\n");
+		return ret;
+	}
+
+	ret = pm_runtime_set_active(dev);
+	if (ret < 0)
+		return ret;
+
+	ret = devm_pm_runtime_enable(dev);
+	if (ret)
+		return ret;
+
+	pm_runtime_get_noresume(dev);
+	pm_runtime_set_autosuspend_delay(dev, 200);
+	pm_runtime_use_autosuspend(dev);
+
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
+	ret = devm_iio_device_register(dev, indio_dev);
+	if (ret)
+		return dev_err_probe(dev, ret, "device register failed\n");
+
+	return 0;
+}
+
+static int als31300_runtime_suspend(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct als31300_data *data = iio_priv(indio_dev);
+	int ret;
+
+	ret = als31300_set_operating_mode(data, ALS31300_VOLATILE_MODE_SLEEP_MODE);
+	if (ret)
+		dev_err(dev, "failed to power off device (%pe)\n", ERR_PTR(ret));
+
+	return ret;
+}
+
+static int als31300_runtime_resume(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct als31300_data *data = iio_priv(indio_dev);
+	int ret;
+
+	ret = als31300_set_operating_mode(data, ALS31300_VOLATILE_MODE_ACTIVE_MODE);
+	if (ret)
+		dev_err(dev, "failed to power on device (%pe)\n", ERR_PTR(ret));
+
+	return ret;
+}
+
+static DEFINE_RUNTIME_DEV_PM_OPS(als31300_pm_ops,
+				 als31300_runtime_suspend, als31300_runtime_resume,
+				 NULL);
+
+static const struct i2c_device_id als31300_id[] = {
+	{ "als31300-500" },
+	{ "als31300-1000" },
+	{ "als31300-2000" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(i2c, als31300_id);
+
+static const struct of_device_id als31300_of_match[] = {
+	{ .compatible = "allegromicro,als31300-500", .data = (void *)4 },
+	{ .compatible = "allegromicro,als31300-1000", .data = (void *)2 },
+	{ .compatible = "allegromicro,als31300-2000", .data = (void *)1 },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, als31300_of_match);
+
+static struct i2c_driver als31300_driver = {
+	.driver	 = {
+		.name = "als31300",
+		.of_match_table = als31300_of_match,
+		.pm = pm_ptr(&als31300_pm_ops),
+	},
+	.probe = als31300_probe,
+	.id_table = als31300_id,
+};
+module_i2c_driver(als31300_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("ALS31300 3-D Linear Hall Effect Driver");
+MODULE_AUTHOR("Neil Armstrong <neil.armstrong@linaro.org>");

-- 
2.34.1

On Mon, 07 Oct 2024 15:14:40 +0200
Neil Armstrong <neil.armstrong@linaro.org> wrote:

> The Allegro MicroSystems ALS31300 is a 3-D Linear Hall Effect Sensor
> mainly used for 3D head-on motion sensing applications.
> 
> The device is configured over I2C, and as part of the Sensor
> data the temperature core is also provided.
> 
> While the device provides an IRQ gpio, it depends on a configuration
> programmed into the internal EEPROM, thus only the default mode
> is supported and buffered input via trigger is also supported
> to allow streaming values with the same sensing timestamp.
> 
> The device can be configured with different sensitivities in factory,
> but the sensitivity value used to calculate value into the Gauss
> unit is not available from registers, thus the sensitivity is
> provided by the compatible/device-id string which is based
> on the part number as described in the datasheet page 2.
> 
> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
Hi Neil.

Pretty clean driver. Just a few minor comments inline.

Thanks,

Jonathan


> diff --git a/drivers/iio/magnetometer/als31300.c b/drivers/iio/magnetometer/als31300.c
> new file mode 100644
> index 000000000000..123e6a63b516
> --- /dev/null
> +++ b/drivers/iio/magnetometer/als31300.c
> @@ -0,0 +1,459 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Driver for the Allegro MicroSystems ALS31300 3-D Linear Hall Effect Sensor
> + *
> + * Copyright (c) 2024 Linaro Limited
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/delay.h>
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/regmap.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
> +
> +/*
> + * The Allegro MicroSystems ALS31300 has an EEPROM space to configure how
> + * the device works and how the interrupt line behaves,

behaves.

> + * we only support the default setup with external trigger

	Only the default setup with external trigger is supported.
etc.

> + *
> + * Since by default the interrupt line is disable, we don't
> + * support GPIO interrupt events for now.
> + *
> + * It should be possible to adapt the driver to the current
> + * device setup, but we leave it as a future exercise.
> + */
> +
> +#define ALS31300_EEPROM_CONFIG		0x02
> +#define ALS31300_EEPROM_INTERRUPT	0x03
> +#define ALS31300_EEPROM_CUSTOMER_1	0x0d
> +#define ALS31300_EEPROM_CUSTOMER_2	0x0e
> +#define ALS31300_EEPROM_CUSTOMER_3	0x0f
> +#define ALS31300_VOLATILE_MODE		0x27

Is spelling out volatile needed? Maybe VOL or just V or skip
it completely as it makes for some long lines?

> +#define ALS31300_VOLATILE_MODE_LPDCM		GENMASK(6, 4)
> +#define ALS31300_VOLATILE_MODE_SLEEP		GENMASK(1, 0)
> +#define ALS31300_VOLATILE_MSB		0x28
> +#define ALS31300_VOLATILE_MSB_TEMPERATURE	GENMASK(5, 0)
> +#define ALS31300_VOLATILE_MSB_INTERRUPT		BIT(6)
> +#define ALS31300_VOLATILE_MSB_NEW_DATA		BIT(7)
> +#define ALS31300_VOLATILE_MSB_Z_AXIS		GENMASK(15, 8)
> +#define ALS31300_VOLATILE_MSB_Y_AXIS		GENMASK(23, 16)
> +#define ALS31300_VOLATILE_MSB_X_AXIS		GENMASK(31, 24)
> +#define ALS31300_VOLATILE_LSB		0x29
> +#define ALS31300_VOLATILE_LSB_TEMPERATURE	GENMASK(5, 0)
> +#define ALS31300_VOLATILE_LSB_HALL_STATUS	GENMASK(7, 7)
> +#define ALS31300_VOLATILE_LSB_Z_AXIS		GENMASK(11, 8)
> +#define ALS31300_VOLATILE_LSB_Y_AXIS		GENMASK(15, 12)
> +#define ALS31300_VOLATILE_LSB_X_AXIS		GENMASK(19, 16)
> +#define ALS31300_VOLATILE_LSB_INTERRUPT_WRITE	BIT(20)
> +#define ALS31300_CUSTOMER_ACCESS	0x35
> +
> +#define ALS31300_LPDCM_INACTIVE_0_5_MS		0
> +#define ALS31300_LPDCM_INACTIVE_1_0_MS		1
> +#define ALS31300_LPDCM_INACTIVE_5_0_MS		2
> +#define ALS31300_LPDCM_INACTIVE_10_0_MS		3
> +#define ALS31300_LPDCM_INACTIVE_50_0_MS		4
> +#define ALS31300_LPDCM_INACTIVE_100_0_MS	5
> +#define ALS31300_LPDCM_INACTIVE_500_0_MS	6
> +#define ALS31300_LPDCM_INACTIVE_1000_0_MS	7
I'd move these up to next to the field def above.
Can play games with indent to make it clear they are the contents of
that field.

#define ALS31300_VOLATILE_MODE_LPDCM		GENMASK(6, 4)
#define   ALS31300_LPDCM_INACTIVE_0_5_MS	0
etc


> +
> +#define ALS31300_VOLATILE_MODE_ACTIVE_MODE	0
> +#define ALS31300_VOLATILE_MODE_SLEEP_MODE	1
> +#define ALS31300_VOLATILE_MODE_LPDCM_MODE	2
> +
> +#define ALS31300_DATA_X_GET(__buf)			\

Why __buf?  I'd just use b

> +		((int)(s8)FIELD_GET(ALS31300_VOLATILE_MSB_X_AXIS, __buf[0]) << 4 | \
> +			  FIELD_GET(ALS31300_VOLATILE_LSB_X_AXIS, __buf[1]))
> +#define ALS31300_DATA_Y_GET(__buf)			\
> +		((int)(s8)FIELD_GET(ALS31300_VOLATILE_MSB_Y_AXIS, __buf[0]) << 4 | \
> +			  FIELD_GET(ALS31300_VOLATILE_LSB_Y_AXIS, __buf[1]))
> +#define ALS31300_DATA_Z_GET(__buf)			\
> +		((int)(s8)FIELD_GET(ALS31300_VOLATILE_MSB_Z_AXIS, __buf[0]) << 4 | \
> +			  FIELD_GET(ALS31300_VOLATILE_LSB_Z_AXIS, __buf[1]))

Nice way to make these more readable is sign_extend32() rather than the casts.
So
	sign_extend32(FIELD_GET(ALS31300_VOLATILE_MSB_X_AXIS, b[0]) << 4 |
		      FIELD_GET(ALS31300_VOLATILE_LSB_X_AXIS, b[1]),
		      11);


> +#define ALS31300_TEMPERATURE_GET(__buf)			\
> +		((u32)(u8)FIELD_GET(ALS31300_VOLATILE_MSB_TEMPERATURE, __buf[0]) << 6 | \
> +			  FIELD_GET(ALS31300_VOLATILE_LSB_TEMPERATURE, __buf[1]))

What does the u8 cast change?

> +

> +struct als31300_data {
> +	struct device *dev;
> +	/* protects power on/off the device and access HW */
> +	struct mutex mutex;
> +	unsigned long sensitivity;
> +	struct regmap *map;
> +	struct {
> +		u16 temperature;
> +		s16 channels[3];
> +		s64 timestamp __aligned(8);
aligned_s64 timestamp


It's new so for now only in the togreg branch of iio.git.

> +	} scan;
> +};
> +
> +/* The whole measure is split into 2x32bit registers, we need to read them both at once */
> +static int als31300_get_measure(struct als31300_data *data, s16 *t, s16 *x,
> +				s16 *y, s16 *z)
> +{
> +	unsigned int count = 0;
> +	u32 buf[2];
> +	int ret;
> +
> +	mutex_lock(&data->mutex);

	guard(mutex)(&data->mutex) and drop the unlock handling.
It's a small simplification but still nice to have here.

> +	ret = pm_runtime_resume_and_get(data->dev);
> +	if (ret)
> +		goto unlock;
> +
> +	/* Max update rate it 2KHz, wait up to 1ms */
> +	while (count < 50) {
> +		/* Read Data */
> +		ret = regmap_bulk_read(data->map, ALS31300_VOLATILE_MSB, buf, 2);
> +		if (ret) {
> +			dev_err(data->dev, "read data failed, error %d\n", ret);
> +			goto out;
> +		}
> +
> +		/* Check if data is valid, happens right after getting out of sleep mode */
> +		if (FIELD_GET(ALS31300_VOLATILE_MSB_NEW_DATA, buf[0]))
> +			break;
> +
> +		usleep_range(10, 20);
> +		++count;
> +	}
> +
> +	if (count >= 50) {
> +		ret = -ETIMEDOUT;
> +		goto out;
> +	}
> +
> +	*t = ALS31300_TEMPERATURE_GET(buf);
> +	*x = ALS31300_DATA_X_GET(buf);
> +	*y = ALS31300_DATA_Y_GET(buf);
> +	*z = ALS31300_DATA_Z_GET(buf);
> +
> +out:
> +	pm_runtime_mark_last_busy(data->dev);
> +	pm_runtime_put_autosuspend(data->dev);
> +
> +unlock:
> +	mutex_unlock(&data->mutex);
> +
> +	return ret;
> +}
> +
> +static int als31300_read_raw(struct iio_dev *indio_dev,
> +			     const struct iio_chan_spec *chan, int *val,
> +			     int *val2, long mask)
> +{
> +	struct als31300_data *data = iio_priv(indio_dev);
> +	s16 t, x, y, z;
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_PROCESSED:
> +	case IIO_CHAN_INFO_RAW:
> +		ret = als31300_get_measure(data, &t, &x, &y, &z);
> +		if (ret)
> +			return ret;

blank line here would perhaps make this a tiny bit easier to read.

> +		switch (chan->address) {
> +		case TEMPERATURE:
> +			*val = t;
> +			return IIO_VAL_INT;
> +		case AXIS_X:
> +			*val = x;
> +			return IIO_VAL_INT;
> +		case AXIS_Y:
> +			*val = y;
> +			return IIO_VAL_INT;
> +		case AXIS_Z:
> +			*val = z;
> +			return IIO_VAL_INT;
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->type) {
> +		case IIO_TEMP:
> +			/*
> +			 * Fractional part of:
> +			 *         302(value - 1708)
> +			 * temp = ------------------
> +			 *             4096
> +			 * to convert temperature in Celcius

Units in IIO ABI (because we copied hwmon) are millidegrees celcius.
Bad decision a long time back, but we are stuck with it.
See Documentation/ABI/testing/sysfs-bus-iio

> +			 */
> +			*val = 302;
> +			*val2 = 4096;
> +			return IIO_VAL_FRACTIONAL;
> +		case IIO_MAGN:
> +			/*
> +			 * Devices are configured in factory
> +			 * with different sensitivities:
> +			 * - 500 GAUSS <-> 4 LSB/Gauss
> +			 * - 1000 GAUSS <-> 2 LSB/Gauss
> +			 * - 2000 GAUSS <-> 1 LSB/Gauss
> +			 * with translates by a division of the returned
> +			 * value to get Gauss value.
> +			 * The sensisitivity cannot be read at runtime
> +			 * so the value depends on the model compatible
> +			 * or device id.
> +			 */
> +			*val = 1;
> +			*val2 = data->sensitivity;
> +			return IIO_VAL_FRACTIONAL;
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_OFFSET:
> +		switch (chan->type) {
> +		case IIO_TEMP:
> +			*val = -1708;
> +			return IIO_VAL_INT;
> +		default:
> +			return -EINVAL;
> +		}
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static irqreturn_t als31300_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct als31300_data *data = iio_priv(indio_dev);
> +	s16 x, y, z;
> +	u16 t;
> +	int ret;
> +
> +	ret = als31300_get_measure(data, &t, &x, &y, &z);
> +	if (ret)
> +		goto trigger_out;
> +
> +	data->scan.temperature = t;
> +	data->scan.channels[0] = x;
> +	data->scan.channels[1] = y;
> +	data->scan.channels[2] = z;

This is pretty small. I'd just put scan on the stack in this function.

> +	iio_push_to_buffers_with_timestamp(indio_dev, &data->scan,
> +					   iio_get_time_ns(indio_dev));

pf->timestamp given you are providing a non threaded interrupt handler
to fill that in.

> +
> +trigger_out:
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}

> +static const struct iio_chan_spec als31300_channels[] = {
> +	{
> +		.type = IIO_TEMP,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +			BIT(IIO_CHAN_INFO_SCALE) |
> +			BIT(IIO_CHAN_INFO_OFFSET),
> +		.address = TEMPERATURE,
> +		.scan_index = TEMPERATURE,
> +		.scan_type = {
> +			.sign = 'u',
> +			.realbits = 16,
> +			.storagebits = 16,
> +			.endianness = IIO_CPU,
> +		},
> +	},
> +	ALS31300_AXIS_CHANNEL(X, AXIS_X),
> +	ALS31300_AXIS_CHANNEL(Y, AXIS_Y),
> +	ALS31300_AXIS_CHANNEL(Z, AXIS_Z),
> +	IIO_CHAN_SOFT_TIMESTAMP(6),

Why 6?

Technically it's not wrong ABI, just odd to leave a gap between the channels
and the timestamp.  Probably wants to be 4

> +};

> +static int als31300_probe(struct i2c_client *i2c)
> +{
> +	struct device *dev = &i2c->dev;
> +	struct als31300_data *data;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +	data->dev = dev;
> +	i2c_set_clientdata(i2c, indio_dev);
> +
> +	mutex_init(&data->mutex);
> +
> +	data->sensitivity = (unsigned long)of_device_get_match_data(dev);
After changing the data to pointers to structures below use
i2c_get_match_data() That will try various types of firmware and fall
back to the id tables if appropriate.

> +
> +	data->map = devm_regmap_init_i2c(i2c, &als31300_regmap_config);
> +	if (IS_ERR(data->map))
> +		return dev_err_probe(dev, PTR_ERR(data->map),
> +				     "failed to allocate register map\n");

...


> +
> +static DEFINE_RUNTIME_DEV_PM_OPS(als31300_pm_ops,
> +				 als31300_runtime_suspend, als31300_runtime_resume,
> +				 NULL);
> +
> +static const struct i2c_device_id als31300_id[] = {
> +	{ "als31300-500" },

This needs data as well because you can probe via the sysfs interface instead
of DT which will use these ids.

> +	{ "als31300-1000" },
> +	{ "als31300-2000" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(i2c, als31300_id);
> +
> +static const struct of_device_id als31300_of_match[] = {
> +	{ .compatible = "allegromicro,als31300-500", .data = (void *)4 },
> +	{ .compatible = "allegromicro,als31300-1000", .data = (void *)2 },
> +	{ .compatible = "allegromicro,als31300-2000", .data = (void *)1 },

Use pointers to structures and also use them above.  Even if those structures
have just one value in them for now.

Just have something like

struct als31300_variant_info {
	u8 sensitivity;
};

static const struct als31300_variant_info al31300_variant_500 = {
	.sensitivity = 4;
};

etc.


> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, als31300_of_match);
> +
> +static struct i2c_driver als31300_driver = {
> +	.driver	 = {
> +		.name = "als31300",
> +		.of_match_table = als31300_of_match,
> +		.pm = pm_ptr(&als31300_pm_ops),
> +	},
> +	.probe = als31300_probe,
> +	.id_table = als31300_id,
> +};
> +module_i2c_driver(als31300_driver);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("ALS31300 3-D Linear Hall Effect Driver");
> +MODULE_AUTHOR("Neil Armstrong <neil.armstrong@linaro.org>");
>

Hi Jonathan,

On 12/10/2024 16:50, Jonathan Cameron wrote:
> On Mon, 07 Oct 2024 15:14:40 +0200
> Neil Armstrong <neil.armstrong@linaro.org> wrote:
> 
>> The Allegro MicroSystems ALS31300 is a 3-D Linear Hall Effect Sensor
>> mainly used for 3D head-on motion sensing applications.
>>
>> The device is configured over I2C, and as part of the Sensor
>> data the temperature core is also provided.
>>
>> While the device provides an IRQ gpio, it depends on a configuration
>> programmed into the internal EEPROM, thus only the default mode
>> is supported and buffered input via trigger is also supported
>> to allow streaming values with the same sensing timestamp.
>>
>> The device can be configured with different sensitivities in factory,
>> but the sensitivity value used to calculate value into the Gauss
>> unit is not available from registers, thus the sensitivity is
>> provided by the compatible/device-id string which is based
>> on the part number as described in the datasheet page 2.
>>
>> Signed-off-by: Neil Armstrong <neil.armstrong@linaro.org>
> Hi Neil.
> 
> Pretty clean driver. Just a few minor comments inline.
> 
> Thanks,
> 
> Jonathan
> 
> 
>> diff --git a/drivers/iio/magnetometer/als31300.c b/drivers/iio/magnetometer/als31300.c
>> new file mode 100644
>> index 000000000000..123e6a63b516
>> --- /dev/null
>> +++ b/drivers/iio/magnetometer/als31300.c
>> @@ -0,0 +1,459 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Driver for the Allegro MicroSystems ALS31300 3-D Linear Hall Effect Sensor
>> + *
>> + * Copyright (c) 2024 Linaro Limited
>> + */
>> +
>> +#include <linux/bitfield.h>
>> +#include <linux/bits.h>
>> +#include <linux/delay.h>
>> +#include <linux/module.h>
>> +#include <linux/i2c.h>
>> +#include <linux/regmap.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/regulator/consumer.h>
>> +
>> +#include <linux/iio/buffer.h>
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/trigger_consumer.h>
>> +#include <linux/iio/triggered_buffer.h>
>> +
>> +/*
>> + * The Allegro MicroSystems ALS31300 has an EEPROM space to configure how
>> + * the device works and how the interrupt line behaves,
> 
> behaves.
> 
>> + * we only support the default setup with external trigger
> 
> 	Only the default setup with external trigger is supported.
> etc.

Ack, I'll rephrase that part

> 
>> + *
>> + * Since by default the interrupt line is disable, we don't
>> + * support GPIO interrupt events for now.
>> + *
>> + * It should be possible to adapt the driver to the current
>> + * device setup, but we leave it as a future exercise.
>> + */
>> +
>> +#define ALS31300_EEPROM_CONFIG		0x02
>> +#define ALS31300_EEPROM_INTERRUPT	0x03
>> +#define ALS31300_EEPROM_CUSTOMER_1	0x0d
>> +#define ALS31300_EEPROM_CUSTOMER_2	0x0e
>> +#define ALS31300_EEPROM_CUSTOMER_3	0x0f
>> +#define ALS31300_VOLATILE_MODE		0x27
> 
> Is spelling out volatile needed? Maybe VOL or just V or skip
> it completely as it makes for some long lines?

Yep, there's no formal naming on the datasheet for those so I'll
reduce to _VOL

> 
>> +#define ALS31300_VOLATILE_MODE_LPDCM		GENMASK(6, 4)
>> +#define ALS31300_VOLATILE_MODE_SLEEP		GENMASK(1, 0)
>> +#define ALS31300_VOLATILE_MSB		0x28
>> +#define ALS31300_VOLATILE_MSB_TEMPERATURE	GENMASK(5, 0)
>> +#define ALS31300_VOLATILE_MSB_INTERRUPT		BIT(6)
>> +#define ALS31300_VOLATILE_MSB_NEW_DATA		BIT(7)
>> +#define ALS31300_VOLATILE_MSB_Z_AXIS		GENMASK(15, 8)
>> +#define ALS31300_VOLATILE_MSB_Y_AXIS		GENMASK(23, 16)
>> +#define ALS31300_VOLATILE_MSB_X_AXIS		GENMASK(31, 24)
>> +#define ALS31300_VOLATILE_LSB		0x29
>> +#define ALS31300_VOLATILE_LSB_TEMPERATURE	GENMASK(5, 0)
>> +#define ALS31300_VOLATILE_LSB_HALL_STATUS	GENMASK(7, 7)
>> +#define ALS31300_VOLATILE_LSB_Z_AXIS		GENMASK(11, 8)
>> +#define ALS31300_VOLATILE_LSB_Y_AXIS		GENMASK(15, 12)
>> +#define ALS31300_VOLATILE_LSB_X_AXIS		GENMASK(19, 16)
>> +#define ALS31300_VOLATILE_LSB_INTERRUPT_WRITE	BIT(20)
>> +#define ALS31300_CUSTOMER_ACCESS	0x35
>> +
>> +#define ALS31300_LPDCM_INACTIVE_0_5_MS		0
>> +#define ALS31300_LPDCM_INACTIVE_1_0_MS		1
>> +#define ALS31300_LPDCM_INACTIVE_5_0_MS		2
>> +#define ALS31300_LPDCM_INACTIVE_10_0_MS		3
>> +#define ALS31300_LPDCM_INACTIVE_50_0_MS		4
>> +#define ALS31300_LPDCM_INACTIVE_100_0_MS	5
>> +#define ALS31300_LPDCM_INACTIVE_500_0_MS	6
>> +#define ALS31300_LPDCM_INACTIVE_1000_0_MS	7
> I'd move these up to next to the field def above.
> Can play games with indent to make it clear they are the contents of
> that field.
> 
> #define ALS31300_VOLATILE_MODE_LPDCM		GENMASK(6, 4)
> #define   ALS31300_LPDCM_INACTIVE_0_5_MS	0
> etc

If it's accepted, then I'll do it!

> 
> 
>> +
>> +#define ALS31300_VOLATILE_MODE_ACTIVE_MODE	0
>> +#define ALS31300_VOLATILE_MODE_SLEEP_MODE	1
>> +#define ALS31300_VOLATILE_MODE_LPDCM_MODE	2
>> +
>> +#define ALS31300_DATA_X_GET(__buf)			\
> 
> Why __buf?  I'd just use b

Right

> 
>> +		((int)(s8)FIELD_GET(ALS31300_VOLATILE_MSB_X_AXIS, __buf[0]) << 4 | \
>> +			  FIELD_GET(ALS31300_VOLATILE_LSB_X_AXIS, __buf[1]))
>> +#define ALS31300_DATA_Y_GET(__buf)			\
>> +		((int)(s8)FIELD_GET(ALS31300_VOLATILE_MSB_Y_AXIS, __buf[0]) << 4 | \
>> +			  FIELD_GET(ALS31300_VOLATILE_LSB_Y_AXIS, __buf[1]))
>> +#define ALS31300_DATA_Z_GET(__buf)			\
>> +		((int)(s8)FIELD_GET(ALS31300_VOLATILE_MSB_Z_AXIS, __buf[0]) << 4 | \
>> +			  FIELD_GET(ALS31300_VOLATILE_LSB_Z_AXIS, __buf[1]))
> 
> Nice way to make these more readable is sign_extend32() rather than the casts.
> So
> 	sign_extend32(FIELD_GET(ALS31300_VOLATILE_MSB_X_AXIS, b[0]) << 4 |
> 		      FIELD_GET(ALS31300_VOLATILE_LSB_X_AXIS, b[1]),
> 		      11);

Thanks for the suggestion, it's indeed better

> 
> 
>> +#define ALS31300_TEMPERATURE_GET(__buf)			\
>> +		((u32)(u8)FIELD_GET(ALS31300_VOLATILE_MSB_TEMPERATURE, __buf[0]) << 6 | \
>> +			  FIELD_GET(ALS31300_VOLATILE_LSB_TEMPERATURE, __buf[1]))
> 
> What does the u8 cast change?

It was to align with the s8 defines, but it won't be needed with sign_extend32

> 
>> +
> 
>> +struct als31300_data {
>> +	struct device *dev;
>> +	/* protects power on/off the device and access HW */
>> +	struct mutex mutex;
>> +	unsigned long sensitivity;
>> +	struct regmap *map;
>> +	struct {
>> +		u16 temperature;
>> +		s16 channels[3];
>> +		s64 timestamp __aligned(8);
> aligned_s64 timestamp
> 
> 
> It's new so for now only in the togreg branch of iio.git.

Ack, I'll use it and rebase on your togreg branch

> 
>> +	} scan;
>> +};
>> +
>> +/* The whole measure is split into 2x32bit registers, we need to read them both at once */
>> +static int als31300_get_measure(struct als31300_data *data, s16 *t, s16 *x,
>> +				s16 *y, s16 *z)
>> +{
>> +	unsigned int count = 0;
>> +	u32 buf[2];
>> +	int ret;
>> +
>> +	mutex_lock(&data->mutex);
> 
> 	guard(mutex)(&data->mutex) and drop the unlock handling.
> It's a small simplification but still nice to have here.

Ok, it'll be my first time, hope I'll get it right

> 
>> +	ret = pm_runtime_resume_and_get(data->dev);
>> +	if (ret)
>> +		goto unlock;
>> +
>> +	/* Max update rate it 2KHz, wait up to 1ms */
>> +	while (count < 50) {
>> +		/* Read Data */
>> +		ret = regmap_bulk_read(data->map, ALS31300_VOLATILE_MSB, buf, 2);
>> +		if (ret) {
>> +			dev_err(data->dev, "read data failed, error %d\n", ret);
>> +			goto out;
>> +		}
>> +
>> +		/* Check if data is valid, happens right after getting out of sleep mode */
>> +		if (FIELD_GET(ALS31300_VOLATILE_MSB_NEW_DATA, buf[0]))
>> +			break;
>> +
>> +		usleep_range(10, 20);
>> +		++count;
>> +	}
>> +
>> +	if (count >= 50) {
>> +		ret = -ETIMEDOUT;
>> +		goto out;
>> +	}
>> +
>> +	*t = ALS31300_TEMPERATURE_GET(buf);
>> +	*x = ALS31300_DATA_X_GET(buf);
>> +	*y = ALS31300_DATA_Y_GET(buf);
>> +	*z = ALS31300_DATA_Z_GET(buf);
>> +
>> +out:
>> +	pm_runtime_mark_last_busy(data->dev);
>> +	pm_runtime_put_autosuspend(data->dev);
>> +
>> +unlock:
>> +	mutex_unlock(&data->mutex);
>> +
>> +	return ret;
>> +}
>> +
>> +static int als31300_read_raw(struct iio_dev *indio_dev,
>> +			     const struct iio_chan_spec *chan, int *val,
>> +			     int *val2, long mask)
>> +{
>> +	struct als31300_data *data = iio_priv(indio_dev);
>> +	s16 t, x, y, z;
>> +	int ret;
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_PROCESSED:
>> +	case IIO_CHAN_INFO_RAW:
>> +		ret = als31300_get_measure(data, &t, &x, &y, &z);
>> +		if (ret)
>> +			return ret;
> 
> blank line here would perhaps make this a tiny bit easier to read.

Ack

> 
>> +		switch (chan->address) {
>> +		case TEMPERATURE:
>> +			*val = t;
>> +			return IIO_VAL_INT;
>> +		case AXIS_X:
>> +			*val = x;
>> +			return IIO_VAL_INT;
>> +		case AXIS_Y:
>> +			*val = y;
>> +			return IIO_VAL_INT;
>> +		case AXIS_Z:
>> +			*val = z;
>> +			return IIO_VAL_INT;
>> +		default:
>> +			return -EINVAL;
>> +		}
>> +	case IIO_CHAN_INFO_SCALE:
>> +		switch (chan->type) {
>> +		case IIO_TEMP:
>> +			/*
>> +			 * Fractional part of:
>> +			 *         302(value - 1708)
>> +			 * temp = ------------------
>> +			 *             4096
>> +			 * to convert temperature in Celcius
> 
> Units in IIO ABI (because we copied hwmon) are millidegrees celcius.
> Bad decision a long time back, but we are stuck with it.
> See Documentation/ABI/testing/sysfs-bus-iio

I was wondering, now I have my answer :-)

> 
>> +			 */
>> +			*val = 302;
>> +			*val2 = 4096;
>> +			return IIO_VAL_FRACTIONAL;
>> +		case IIO_MAGN:
>> +			/*
>> +			 * Devices are configured in factory
>> +			 * with different sensitivities:
>> +			 * - 500 GAUSS <-> 4 LSB/Gauss
>> +			 * - 1000 GAUSS <-> 2 LSB/Gauss
>> +			 * - 2000 GAUSS <-> 1 LSB/Gauss
>> +			 * with translates by a division of the returned
>> +			 * value to get Gauss value.
>> +			 * The sensisitivity cannot be read at runtime
>> +			 * so the value depends on the model compatible
>> +			 * or device id.
>> +			 */
>> +			*val = 1;
>> +			*val2 = data->sensitivity;
>> +			return IIO_VAL_FRACTIONAL;
>> +		default:
>> +			return -EINVAL;
>> +		}
>> +	case IIO_CHAN_INFO_OFFSET:
>> +		switch (chan->type) {
>> +		case IIO_TEMP:
>> +			*val = -1708;
>> +			return IIO_VAL_INT;
>> +		default:
>> +			return -EINVAL;
>> +		}
>> +
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>> +static irqreturn_t als31300_trigger_handler(int irq, void *p)
>> +{
>> +	struct iio_poll_func *pf = p;
>> +	struct iio_dev *indio_dev = pf->indio_dev;
>> +	struct als31300_data *data = iio_priv(indio_dev);
>> +	s16 x, y, z;
>> +	u16 t;
>> +	int ret;
>> +
>> +	ret = als31300_get_measure(data, &t, &x, &y, &z);
>> +	if (ret)
>> +		goto trigger_out;
>> +
>> +	data->scan.temperature = t;
>> +	data->scan.channels[0] = x;
>> +	data->scan.channels[1] = y;
>> +	data->scan.channels[2] = z;
> 
> This is pretty small. I'd just put scan on the stack in this function.

Ack

> 
>> +	iio_push_to_buffers_with_timestamp(indio_dev, &data->scan,
>> +					   iio_get_time_ns(indio_dev));
> 
> pf->timestamp given you are providing a non threaded interrupt handler
> to fill that in.

Ok

> 
>> +
>> +trigger_out:
>> +	iio_trigger_notify_done(indio_dev->trig);
>> +
>> +	return IRQ_HANDLED;
>> +}
> 
>> +static const struct iio_chan_spec als31300_channels[] = {
>> +	{
>> +		.type = IIO_TEMP,
>> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>> +			BIT(IIO_CHAN_INFO_SCALE) |
>> +			BIT(IIO_CHAN_INFO_OFFSET),
>> +		.address = TEMPERATURE,
>> +		.scan_index = TEMPERATURE,
>> +		.scan_type = {
>> +			.sign = 'u',
>> +			.realbits = 16,
>> +			.storagebits = 16,
>> +			.endianness = IIO_CPU,
>> +		},
>> +	},
>> +	ALS31300_AXIS_CHANNEL(X, AXIS_X),
>> +	ALS31300_AXIS_CHANNEL(Y, AXIS_Y),
>> +	ALS31300_AXIS_CHANNEL(Z, AXIS_Z),
>> +	IIO_CHAN_SOFT_TIMESTAMP(6),
> 
> Why 6?
> 
> Technically it's not wrong ABI, just odd to leave a gap between the channels
> and the timestamp.  Probably wants to be 4

Seems it's a bad copy-paste and not knowing what 6 was meaning, thx for the clarification

> 
>> +};
> 
>> +static int als31300_probe(struct i2c_client *i2c)
>> +{
>> +	struct device *dev = &i2c->dev;
>> +	struct als31300_data *data;
>> +	struct iio_dev *indio_dev;
>> +	int ret;
>> +
>> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
>> +	if (!indio_dev)
>> +		return -ENOMEM;
>> +
>> +	data = iio_priv(indio_dev);
>> +	data->dev = dev;
>> +	i2c_set_clientdata(i2c, indio_dev);
>> +
>> +	mutex_init(&data->mutex);
>> +
>> +	data->sensitivity = (unsigned long)of_device_get_match_data(dev);
> After changing the data to pointers to structures below use
> i2c_get_match_data() That will try various types of firmware and fall
> back to the id tables if appropriate.

Yep I have a change that does that already, I figured that out after sending v1...

> 
>> +
>> +	data->map = devm_regmap_init_i2c(i2c, &als31300_regmap_config);
>> +	if (IS_ERR(data->map))
>> +		return dev_err_probe(dev, PTR_ERR(data->map),
>> +				     "failed to allocate register map\n");
> 
> ...
> 
> 
>> +
>> +static DEFINE_RUNTIME_DEV_PM_OPS(als31300_pm_ops,
>> +				 als31300_runtime_suspend, als31300_runtime_resume,
>> +				 NULL);
>> +
>> +static const struct i2c_device_id als31300_id[] = {
>> +	{ "als31300-500" },
> 
> This needs data as well because you can probe via the sysfs interface instead
> of DT which will use these ids.
> 
>> +	{ "als31300-1000" },
>> +	{ "als31300-2000" },
>> +	{ /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(i2c, als31300_id);
>> +
>> +static const struct of_device_id als31300_of_match[] = {
>> +	{ .compatible = "allegromicro,als31300-500", .data = (void *)4 },
>> +	{ .compatible = "allegromicro,als31300-1000", .data = (void *)2 },
>> +	{ .compatible = "allegromicro,als31300-2000", .data = (void *)1 },
> 
> Use pointers to structures and also use them above.  Even if those structures
> have just one value in them for now.
> 
> Just have something like
> 
> struct als31300_variant_info {
> 	u8 sensitivity;
> };
> 
> static const struct als31300_variant_info al31300_variant_500 = {
> 	.sensitivity = 4;
> };
> 
> etc.

Yep I'll switch to that

> 
> 
>> +	{ /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, als31300_of_match);
>> +
>> +static struct i2c_driver als31300_driver = {
>> +	.driver	 = {
>> +		.name = "als31300",
>> +		.of_match_table = als31300_of_match,
>> +		.pm = pm_ptr(&als31300_pm_ops),
>> +	},
>> +	.probe = als31300_probe,
>> +	.id_table = als31300_id,
>> +};
>> +module_i2c_driver(als31300_driver);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_DESCRIPTION("ALS31300 3-D Linear Hall Effect Driver");
>> +MODULE_AUTHOR("Neil Armstrong <neil.armstrong@linaro.org>");
>>
> 

Thanks,
Neil