Re: [PATCH v3 2/2] iio: magnetometer: si7210: add driver for Si7210

[Jonathan Cameron <jic23@kernel.org>]

On Sun, 12 Jan 2025 11:44:53 +0100
Antoni Pokusinski <apokusinski01@gmail.com> wrote:

> Silicon Labs Si7210 is an I2C Hall effect magnetic position and
> temperature sensor. The driver supports the following functionalities:
> * reading the temperature measurements
> * reading the magnetic field measurements in a single-shot mode
> * choosing the magnetic field measurement scale (20 or 200 mT)
> 
> Signed-off-by: Antoni Pokusinski <apokusinski01@gmail.com>
Hi Antoni,

Some issues in the endian handling as your fix for the build bot
warnings is backwards I think.
A few other comments inline.

Jonathan

> diff --git a/drivers/iio/magnetometer/si7210.c b/drivers/iio/magnetometer/si7210.c
> new file mode 100644
> index 000000000000..107312d127e6
> --- /dev/null
> +++ b/drivers/iio/magnetometer/si7210.c
> @@ -0,0 +1,428 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Silicon Labs Si7210 Hall Effect sensor driver
> + *
> + * Copyright (c) 2024 Antoni Pokusinski <apokusinski01@gmail.com>
> + *
> + * Datasheet:
> + *  https://www.silabs.com/documents/public/data-sheets/si7210-datasheet.pdf
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/i2c.h>
> +#include <linux/iio/iio.h>
> +#include <linux/math64.h>
> +#include <linux/mutex.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>

At lease linux/mod_devicetable.h is missing here.
we  more or less go by include what you use in kernel drivers so
you should make minimal assumptions about what includes what.
There are a few headers that are subsections of a wider interface
where that isn't necessary but for most things should be here.


> +static const struct iio_chan_spec si7210_channels[] = {
> +	{
> +		.type = IIO_MAGN,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +			BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFSET),
> +	},
> +	{

Slight preference for more compact
	}, {


> +		.type = IIO_TEMP,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),
> +	}
> +};
> +
> +static int si7210_fetch_measurement(struct si7210_data *data,
> +				    struct iio_chan_spec const *chan,
> +				    __be16 *buf)
> +{
> +	u8 dspsigsel = chan->type == IIO_MAGN ? 0 : 1;
> +	int ret, result;
> +
> +	guard(mutex)(&data->fetch_lock);
> +
> +	ret = regmap_update_bits(data->regmap, SI7210_REG_DSPSIGSEL,
> +				 SI7210_MASK_DSPSIGSEL, dspsigsel);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_update_bits(data->regmap, SI7210_REG_POWER_CTRL,
> +				 SI7210_MASK_ONEBURST | SI7210_MASK_STOP,
> +				 SI7210_MASK_ONEBURST & ~SI7210_MASK_STOP);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Read the contents of the registers containing the result: DSPSIGM, DSPSIGL */
> +	ret = regmap_bulk_read(data->regmap, SI7210_REG_DSPSIGM, &result, 2);
use sizeof to replace that 2.

> +	if (ret < 0)
> +		return ret;
> +
> +	*buf = cpu_to_be16(result);

You've lost me.  The regmap bulk will load in 'an order'. Not sure whether thant
is big endian or little endian here, but it's definitely not CPU endian.
I 'think' you can read directly into buf.  Must be a wrong endian conversion
somewhere though as on typical le system you are currently reversing the bytes
here and at the callsite.

However, I think what you actually want is
static int si7210_fetch_measurement(struct si7210_data *data,
				    struct iio_chan_spec const *chan,
				    u16 *buf)
	__be16 result;
...


	ret = regmap_bulk_read(..., &result, sizeof(result));
...
	*buf = be16_to_cpu(result);





> +
> +	return 0;
> +}
> +
> +static int si7210_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val, int *val2, long mask)
> +{
> +	struct si7210_data *data = iio_priv(indio_dev);
> +	long long temp;
> +	__be16 dspsig;
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = si7210_fetch_measurement(data, chan, &dspsig);
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = dspsig & GENMASK(14, 0);
It is big endian. You can't do this and expect to hit the right bits.
More to the point you can't set val to the big endian anyway so markings
are wrong somewhere.

> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = 0;
> +		if (data->curr_scale == 20)
> +			*val2 = 1250;
> +		else /* data->curr_scale == 200 */
> +			*val2 = 12500;
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	case IIO_CHAN_INFO_OFFSET:
> +		*val = -16384;
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_PROCESSED:
> +		ret = si7210_fetch_measurement(data, chan, &dspsig);
> +		if (ret < 0)
> +			return ret;
> +
Big endian, You can't do any of this safely. Convert it to cpu endian (e.g. u16)
first.
> +		temp = FIELD_GET(GENMASK(14, 3), dspsig);
> +		temp = div_s64(-383 * temp * temp, 100) + 160940 * temp - 279800000;
> +		temp = (1 + (data->temp_gain / 2048)) * temp + (1000000 / 16) * data->temp_offset;
> +
> +		ret = regulator_get_voltage(data->vdd);
> +		if (ret < 0)
> +			return ret;
> +
> +		temp -= 222 * div_s64(ret, 1000);
> +
> +		*val = div_s64(temp, 1000);
> +
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +}
...

> +static int si7210_device_wake(struct si7210_data *data)
> +{
> +	/*
> +	 * According to the datasheet, the primary method to wake up a
> +	 * device is to send an empty write. However this is not feasible
> +	 * using current API so we use the other method i.e. read a single
> +	 * byte. The device should respond with 0xFF.
> +	 */
> +
> +	int ret;
> +
> +	ret = i2c_smbus_read_byte(data->client);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (ret != 0xFF)
> +		return -EIO;
> +
> +	return 0;
> +}
> +
> +static int si7210_device_init(struct si7210_data *data)
> +{
> +	int ret;
> +	unsigned int i;
> +
> +	ret = si7210_device_wake(data);
> +	if (ret < 0)
> +		return ret;
> +
> +	fsleep(1000);
> +
> +	ret = si7210_read_otpreg_val(data, SI7210_OTPREG_TMP_GAIN, &data->temp_gain);
> +	if (ret < 0)
> +		return ret;

Blank line here and similar places where you have a call / check result pair before
doing something more or less unrelated.

> +	ret = si7210_read_otpreg_val(data, SI7210_OTPREG_TMP_OFF, &data->temp_offset);
> +	if (ret < 0)
> +		return ret;
> +
> +	for (i = 0; i < A_REGS_COUNT; i++) {
> +		ret = si7210_read_otpreg_val(data, a20_otp_regs[i], &data->scale_20_a[i]);
> +		if (ret < 0)
> +			return ret;
> +	}
> +	for (i = 0; i < A_REGS_COUNT; i++) {
> +		ret = si7210_read_otpreg_val(data, a200_otp_regs[i], &data->scale_200_a[i]);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	ret = regmap_update_bits(data->regmap, SI7210_REG_ARAUTOINC,
> +				 SI7210_MASK_ARAUTOINC, SI7210_MASK_ARAUTOINC);
> +	if (ret < 0)
> +		return ret;
> +
> +	return si7210_set_scale(data, 20);
> +}
>