Message-ID: <f19cf5b0-39d8-4297-96c5-e00c4f61c8f6@linaro.org>
Date: Fri, 18 Oct 2024 12:40:21 +0200
From: Neil Armstrong <neil.armstrong@...aro.org>
To: Jonathan Cameron <jic23@...nel.org>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Lars-Peter Clausen <lars@...afoo.de>,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-iio@...r.kernel.org
Subject: Re: [PATCH 3/3] iio: magnetometer: add Allegro MicroSystems ALS31300
 3-D Linear Hall Effect driver

Hi Jonathan,

On 12/10/2024 16:50, Jonathan Cameron wrote:
> On Mon, 07 Oct 2024 15:14:40 +0200
> Neil Armstrong <neil.armstrong@...aro.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@...aro.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@...aro.org>");
>>
> 

Thanks,
Neil

Powered by blists - more mailing lists