* Re: [PATCH 2/2] iio: temperature: add ADI MAX30210 driver
2026-02-26 16:30 ` [PATCH 2/2] iio: temperature: add ADI MAX30210 driver John Erasmus Mari Geronimo
@ 2026-02-26 17:48 ` Andy Shevchenko
2026-02-26 20:08 ` David Lechner
` (3 subsequent siblings)
4 siblings, 0 replies; 11+ messages in thread
From: Andy Shevchenko @ 2026-02-26 17:48 UTC (permalink / raw)
To: John Erasmus Mari Geronimo
Cc: linux-iio, devicetree, linux-kernel, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko
On Fri, Feb 27, 2026 at 12:30:41AM +0800, John Erasmus Mari Geronimo wrote:
> MAX30210 ±0.1°C Accurate Ultra-Small Low-Power Digital Temperature Sensor
Not enough for the commit message.
...
> +#include <asm/div64.h>
linux/math64.h
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/debugfs.h>
Used?
> +#include <linux/delay.h>
> +#include <linux/errno.h>
You missed err.h
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/events.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_consumer.h>
Wow! All of them are in use?
> +#include <linux/interrupt.h>
> +#include <linux/log2.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/property.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +#include <linux/stat.h>
> +#include <linux/string.h>
> +#include <linux/unaligned.h>
> +#include <linux/units.h>
...
> +struct max30210_state {
> + /*
> + * Prevent simultaneous access to the i2c client.
> + */
> + struct mutex lock;
> + struct regmap *regmap;
And if you swap them, won't the binary size be less?
> + struct iio_trigger *trig;
> + struct gpio_desc *powerdown_gpio;
> + u8 watermark;
> + u8 data[3 * MAX30210_FIFO_SIZE] __aligned(IIO_DMA_MINALIGN);
Hmm... Don't we have a macro for this nowadays?
> +};
...
> +static const int samp_freq_avail[] = {
Why not 2D array?
> + 0, 15625,
> + 0, 31250,
> + 0, 62500,
> + 0, 125000,
> + 0, 250000,
> + 0, 500000,
> + 1, 0,
> + 2, 0,
> + 4, 0,
> + 8, 0
Leave trailing comma, it's not a terminator.
> +};
...
> +static int max30210_read_temp(struct regmap *regmap, unsigned int reg,
> + int *temp)
> +{
> + u8 uval[2] __aligned(IIO_DMA_MINALIGN);
No way. This is variable on stack, not all CPUs / architectures allow this.
And actually why this alignment to begin with? Wouldn't
__be16 val;
suffice?
> + int ret;
> +
> + ret = regmap_bulk_read(regmap, reg, uval, 2);
sizeof()
> + if (ret)
> + return ret;
> +
> + *temp = sign_extend32(get_unaligned_be16(uval), 15);
> +
> + return IIO_VAL_INT;
> +}
...
> +static void max30210_fifo_read(struct iio_dev *indio_dev)
> +{
> + struct max30210_state *st = iio_priv(indio_dev);
> + u32 samp;
> + int ret, i, j;
Why are 'i' and 'j' signed?
> + ret = regmap_bulk_read(st->regmap, MAX30210_FIFO_DATA_REG,
> + st->data, 3 * st->watermark);
> + if (ret < 0)
> + return dev_err(&indio_dev->dev, "Failed to read from fifo.\n");
> +
> + for (i = 0; i < st->watermark; i++) {
'i' is not used outside for-loop, hence
for (unsigned int i = 0; i < st->watermark; i++) {
> + samp = 0;
> + for (j = 0; j < 3; j++) {
> + samp <<= 8;
> + samp |= st->data[3 * i + j];
> + }
Reinventing get_unaligned_be32() if I'm not mistaken.
> + if (samp == MAX30210_FIFO_INVAL_DATA) {
> + dev_err(&indio_dev->dev, "Invalid data\n");
> + continue;
> + }
> +
> + iio_push_to_buffers(indio_dev, &samp);
> + }
> +}
...
> +static int max30210_setup(struct max30210_state *st, struct device *dev)
> +{
> + unsigned int val;
> +
> + /* Power down to reset device */
> + st->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
> + GPIOD_OUT_HIGH);
> + if (IS_ERR(st->powerdown_gpio))
> + return dev_err_probe(dev, PTR_ERR(st->powerdown_gpio),
> + "Failed to request powerdown GPIO.\n");
> +
> + /* Power up device */
> + gpiod_set_value(st->powerdown_gpio, 0);
All delays must be documented. Add a comment with the datasheet reference to
explain the value and need of the sleep.
> + fsleep(700);
> + /* Clear status byte */
> + return regmap_read(st->regmap, MAX30210_STATUS_REG, &val);
> +}
...
> +static int max30210_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct iio_dev *indio_dev;
> + struct max30210_state *st;
> + int ret;
> +
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA))
> + return -EOPNOTSUPP;
> +
> + indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + st = iio_priv(indio_dev);
> + mutex_init(&st->lock);
ret = devm_mutex_init(...);
> + ret = devm_regulator_get_enable(dev, "vdd");
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Failed to enable vdd regulator.\n");
> +
> + st->regmap = devm_regmap_init_i2c(client, &max30210_regmap);
> + if (IS_ERR(st->regmap))
> + return dev_err_probe(dev, PTR_ERR(st->regmap),
> + "Failed to allocate regmap.\n");
> +
> + ret = max30210_setup(st, dev);
> + if (ret)
> + return ret;
> +
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->channels = &max30210_channels;
> + indio_dev->num_channels = 1;
> + indio_dev->name = "max30210";
> + indio_dev->info = &max30210_info;
> +
> + ret = devm_iio_triggered_buffer_setup_ext(dev, indio_dev, NULL,
> + max30210_trigger_handler,
> + IIO_BUFFER_DIRECTION_IN,
> + &max30210_buffer_ops,
> + max30210_fifo_attributes);
> + if (ret < 0)
> + return ret;
> +
> + if (client->irq) {
> + st->trig = devm_iio_trigger_alloc(dev, "%s-dev%d",
> + indio_dev->name,
> + iio_device_id(indio_dev));
> + if (!st->trig)
> + return -ENOMEM;
> +
> + st->trig->ops = &max30210_trigger_ops;
> + iio_trigger_set_drvdata(st->trig, indio_dev);
> + ret = devm_iio_trigger_register(dev, st->trig);
> + if (ret)
> + return ret;
> +
> + indio_dev->trig = st->trig;
> + ret = devm_request_threaded_irq(dev, client->irq,
> + iio_trigger_generic_data_rdy_poll,
> + NULL, IRQF_TRIGGER_FALLING,
> + indio_dev->name, st->trig);
> + if (ret)
> + return ret;
> + }
> + ret = devm_iio_device_register(dev, indio_dev);
> + if (ret)
> + return ret;
> +
> + return 0;
Wouldn't
return devm_iio_device_register(dev, indio_dev);
suffice?
> +}
...
> +static const struct i2c_device_id max30210_id[] = {
> + { "max30210", 0 },
No ', 0' part.
> + { }
> +};
...
Can somebody at Analog start a common internal Wiki or other resources
and collect there typical requirements for the code in IIO? It will prevent
reviewers and maintainers from doing the same replies again and again.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 2/2] iio: temperature: add ADI MAX30210 driver
2026-02-26 16:30 ` [PATCH 2/2] iio: temperature: add ADI MAX30210 driver John Erasmus Mari Geronimo
2026-02-26 17:48 ` Andy Shevchenko
@ 2026-02-26 20:08 ` David Lechner
2026-02-26 21:26 ` kernel test robot
` (2 subsequent siblings)
4 siblings, 0 replies; 11+ messages in thread
From: David Lechner @ 2026-02-26 20:08 UTC (permalink / raw)
To: John Erasmus Mari Geronimo, linux-iio
Cc: devicetree, linux-kernel, Jonathan Cameron, Nuno Sá,
Andy Shevchenko
On 2/26/26 10:30 AM, John Erasmus Mari Geronimo wrote:
> MAX30210 ±0.1°C Accurate Ultra-Small Low-Power Digital Temperature Sensor
>
> Signed-off-by: John Erasmus Mari Geronimo <johnerasmusmari.geronimo@analog.com>
> ---
> MAINTAINERS | 8 +
> drivers/iio/temperature/Kconfig | 10 +
> drivers/iio/temperature/Makefile | 1 +
> drivers/iio/temperature/max30210.c | 758 +++++++++++++++++++++++++++++
> 4 files changed, 777 insertions(+)
> create mode 100644 drivers/iio/temperature/max30210.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1c75276404df..2abdbcf3a8e5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1638,6 +1638,14 @@ W: https://ez.analog.com/linux-software-drivers
> F: Documentation/devicetree/bindings/iio/dac/adi,max22007.yaml
> F: drivers/iio/dac/max22007.c
>
> +ANALOG DEVICES INC MAX30210 DRIVER
> +M: John Erasmus Mari Geronimo <johnerasmusmari.geronimo@analog.com>
> +L: linux-iio@vger.kernel.org
> +S: Supported
> +W: https://ez.analog.com/linux-software-drivers
> +F: Documentation/devicetree/bindings/iio/temperature/adi,max30210.yaml
The part above should be introduced in the first patch where the file
above is added.
> +F: drivers/iio/temperature/max30210.c
Then only this part added in this patch.
> +
> ANALOG DEVICES INC ADA4250 DRIVER
> M: Antoniu Miclaus <antoniu.miclaus@analog.com>
> L: linux-iio@vger.kernel.org
> diff --git a/drivers/iio/temperature/Kconfig b/drivers/iio/temperature/Kconfig
> index 9328b2250ace..2d7cb50e2538 100644
> --- a/drivers/iio/temperature/Kconfig
> +++ b/drivers/iio/temperature/Kconfig
> @@ -184,4 +184,14 @@ config MCP9600
> This driver can also be built as a module. If so, the module
> will be called mcp9600.
>
> +config MAX30210
> + tristate "MAX30210 Low-Power I2C Digital Temperature Sensor"
> + depends on I2C
> + help
> + If you say yes here you get support for MAX30210 low-power digital
> + temperature sensor chip connected via I2C.
> +
> + This driver can also be build as a module. If so, the module
> + will be called max30210.
> +
> endmenu
> diff --git a/drivers/iio/temperature/Makefile b/drivers/iio/temperature/Makefile
> index 07d6e65709f7..e5aad14dc09b 100644
> --- a/drivers/iio/temperature/Makefile
> +++ b/drivers/iio/temperature/Makefile
> @@ -8,6 +8,7 @@ obj-$(CONFIG_LTC2983) += ltc2983.o
> obj-$(CONFIG_HID_SENSOR_TEMP) += hid-sensor-temperature.o
> obj-$(CONFIG_MAXIM_THERMOCOUPLE) += maxim_thermocouple.o
> obj-$(CONFIG_MAX30208) += max30208.o
> +obj-$(CONFIG_MAX30210) += max30210.o
I suppose this has enough differences from max30208 and other chips
to justify a separate driver?
> obj-$(CONFIG_MAX31856) += max31856.o
> obj-$(CONFIG_MAX31865) += max31865.o
> obj-$(CONFIG_MCP9600) += mcp9600.o
> diff --git a/drivers/iio/temperature/max30210.c b/drivers/iio/temperature/max30210.c
> new file mode 100644
> index 000000000000..aaa3a26be131
> --- /dev/null
> +++ b/drivers/iio/temperature/max30210.c
> @@ -0,0 +1,758 @@
> +// SPDX-License-Identifier: GPL-2.0
Prefer more precise GPL-2.0-only or GPL-2.0-or-later (your choice).
> +/*
> + * Analog Devices MAX30210 I2C Temperature Sensor driver
> + *
> + * Copyright 2026 Analog Devices Inc.
> + */
> +
> +#include <asm/div64.h>
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/debugfs.h>
> +#include <linux/delay.h>
> +#include <linux/errno.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/events.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/interrupt.h>
> +#include <linux/log2.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/property.h>
> +#include <linux/pwm.h>
> +#include <linux/regmap.h>
> +#include <linux/stat.h>
> +#include <linux/string.h>
> +#include <linux/unaligned.h>
> +#include <linux/units.h>
> +
> +#define MAX30210_STATUS_REG 0x00
> +#define MAX30210_INT_EN_REG 0x02
> +#define MAX30210_FIFO_DATA_REG 0x08
> +#define MAX30210_FIFO_CONF_1_REG 0x09
> +#define MAX30210_FIFO_CONF_2_REG 0x0A
> +#define MAX30210_SYS_CONF_REG 0x11
> +#define MAX30210_PIN_CONF_REG 0x12
> +#define MAX30210_TEMP_ALM_HI_REG 0x22
> +#define MAX30210_TEMP_ALM_LO_REG 0x24
> +#define MAX30210_TEMP_INC_THRESH_REG 0x26
> +#define MAX30210_TEMP_DEC_THRESH_REG 0x27
> +#define MAX30210_TEMP_CONF_1_REG 0x28
> +#define MAX30210_TEMP_CONF_2_REG 0x29
> +#define MAX30210_TEMP_CONV_REG 0x2A
> +#define MAX30210_TEMP_DATA_REG 0x2B
> +#define MAX30210_TEMP_SLOPE_REG 0x2D
> +#define MAX30210_UNIQUE_ID_REG 0x30
> +#define MAX30210_PART_ID_REG 0xFF
> +
> +#define MAX30210_A_FULL_MASK BIT(7)
> +#define MAX30210_TEMP_RDY_MASK BIT(6)
> +#define MAX30210_TEMP_DEC_MASK BIT(5)
> +#define MAX30210_TEMP_INC_MASK BIT(4)
> +#define MAX30210_TEMP_LO_MASK BIT(3)
> +#define MAX30210_TEMP_HI_MASK BIT(2)
> +#define MAX30210_PWR_RDY_MASK BIT(0)
> +
> +#define MAX30210_FLUSH_FIFO_MASK BIT(4)
> +
> +#define MAX30210_EXT_CNV_EN_MASK BIT(7)
> +#define MAX30210_EXT_CVT_ICFG_MASK BIT(6)
> +#define MAX30210_INT_FCFG_MASK GENMASK(3, 2)
> +#define MAX30210_INT_OCFG_MASK GENMASK(1, 0)
> +
> +#define MAX30210_CHG_DET_EN_MASK BIT(3)
> +#define MAX30210_RATE_CHG_FILTER_MASK GENMASK(2, 0)
> +
> +#define MAX30210_TEMP_PERIOD_MASK GENMASK(3, 0)
> +#define MAX30210_ALERT_MODE_MASK BIT(7)
> +
> +#define MAX30210_AUTO_MASK BIT(1)
> +#define MAX30210_CONV_T_MASK BIT(0)
> +
> +#define MAX30210_PART_ID 0x45
> +#define MAX30210_FIFO_SIZE 64
> +#define MAX30210_FIFO_INVAL_DATA GENMASK(23, 0)
> +#define MAX30210_WATERMARK_DEFAULT (0x40 - 0x1F)
> +
> +#define MAX30210_INT_EN(state, mask) ((state) ? (mask) : 0x0)
> +
> +#define MAX30210_UNIQUE_ID_LEN 6
> +#define MAX30210_EXT_CVT_FREQ_MIN 1
> +#define MAX30210_EXT_CVT_FREQ_MAX 20
> +
> +struct max30210_state {
> + /*
> + * Prevent simultaneous access to the i2c client.
This is currently only used in the interrupt handler, which is already
protected agains reentrancy. So we either don't need this or it should
be used in many more places to actually provide protection.
> + */
> + struct mutex lock;
> + struct regmap *regmap;
> + struct iio_trigger *trig;
Probably can be a local varaible when combined with another suggestion
later to use a default function implementation.
> + struct gpio_desc *powerdown_gpio;
This is not used outside of max30210_setup(), so can be a local variable.
> + u8 watermark;
> + u8 data[3 * MAX30210_FIFO_SIZE] __aligned(IIO_DMA_MINALIGN);
We should make this clear that this is FIFO data and not scan data.
Usually, we only see something like this for scan data, which would
need to be 4 bytes per sample rather than 3.
(This is why Andy thought we shuold be using IIO_DECLARE_BUFFER_WITH_TS()
here, but it isn't actually the case.)
> +};
> +
> +static const int samp_freq_avail[] = {
> + 0, 15625,
> + 0, 31250,
> + 0, 62500,
> + 0, 125000,
> + 0, 250000,
> + 0, 500000,
> + 1, 0,
> + 2, 0,
> + 4, 0,
> + 8, 0
> +};
> +
> +static const struct regmap_config max30210_regmap = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .max_register = MAX30210_PART_ID_REG,
> +};
> +
> +static int max30210_read_temp(struct regmap *regmap, unsigned int reg,
> + int *temp)
> +{
> + u8 uval[2] __aligned(IIO_DMA_MINALIGN);
> + int ret;
> +
> + ret = regmap_bulk_read(regmap, reg, uval, 2);
> + if (ret)
> + return ret;
> +
> + *temp = sign_extend32(get_unaligned_be16(uval), 15);
> +
> + return IIO_VAL_INT;
> +}
> +
> +static int max30210_write_temp(struct regmap *regmap, unsigned int reg,
> + int temp)
> +{
> + u8 uval[2] __aligned(IIO_DMA_MINALIGN);
> +
> + put_unaligned_be16(temp, uval);
> +
> + return regmap_bulk_write(regmap, reg, uval, 2);
> +}
> +
> +static void max30210_fifo_read(struct iio_dev *indio_dev)
> +{
> + struct max30210_state *st = iio_priv(indio_dev);
> + u32 samp;
> + int ret, i, j;
> +
> + ret = regmap_bulk_read(st->regmap, MAX30210_FIFO_DATA_REG,
> + st->data, 3 * st->watermark);
> + if (ret < 0)
> + return dev_err(&indio_dev->dev, "Failed to read from fifo.\n");
> +
> + for (i = 0; i < st->watermark; i++) {
> + samp = 0;
> +
> + for (j = 0; j < 3; j++) {
> + samp <<= 8;
> + samp |= st->data[3 * i + j];
> + }
Can we not just do a 3 byte memcpy() here?
Or if this is making it CPU-endian, then use get_unaligned_be24()
and fix IIO_BE to be IIO_CPU in the scan data.
> +
> + if (samp == MAX30210_FIFO_INVAL_DATA) {
> + dev_err(&indio_dev->dev, "Invalid data\n");
This error could get quite noisy. There is a rate-limited version
of dev_err() we could use.
> + continue;
> + }
> +
> + iio_push_to_buffers(indio_dev, &samp);
> + }
> +}
> +
> +static irqreturn_t max30210_trigger_handler(int irq, void *p)
> +{
> + struct iio_poll_func *pf = p;
> + struct iio_dev *indio_dev = pf->indio_dev;
> + struct max30210_state *st = iio_priv(indio_dev);
> + unsigned int status;
> + int ret;
> +
> + guard(mutex)(&st->lock);
> +
> + ret = regmap_read(st->regmap, MAX30210_STATUS_REG, &status);
> + if (ret) {
> + dev_err(&indio_dev->dev, "Status byte read error\n");
> + goto exit_irq;
> + }
> +
> + if (status & MAX30210_PWR_RDY_MASK) {
> + dev_info(&indio_dev->dev, "power-on\n");
dev_dbg(). Although I question that we really need this at all. There is
a time delay in the setup function, so it seems like nothing would be
waiting for this.
> + st->watermark = MAX30210_WATERMARK_DEFAULT;
Would make more sense to set the default during probe.
> + }
> +
> + if (status & MAX30210_A_FULL_MASK)
> + max30210_fifo_read(indio_dev);
> +
> + if (status & MAX30210_TEMP_HI_MASK)
> + iio_push_event(indio_dev,
> + IIO_UNMOD_EVENT_CODE(IIO_TEMP, 0,
> + IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_RISING),
> + iio_get_time_ns(indio_dev));
> +
> + if (status & MAX30210_TEMP_LO_MASK)
> + iio_push_event(indio_dev,
> + IIO_UNMOD_EVENT_CODE(IIO_TEMP, 0,
> + IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_FALLING),
> + iio_get_time_ns(indio_dev));
> +
> +exit_irq:
> + iio_trigger_notify_done(indio_dev->trig);
> + return IRQ_HANDLED;
> +}
> +
> +static int max30210_reg_access(struct iio_dev *indio_dev, unsigned int reg,
> + unsigned int writeval, unsigned int *readval)
> +{
> + struct max30210_state *st = iio_priv(indio_dev);
> +
> + if (!readval)
> + return regmap_write(st->regmap, reg, writeval);
> +
> + return regmap_read(st->regmap, reg, readval);
> +}
> +
> +static int max30210_validate_trigger(struct iio_dev *indio_dev,
> + struct iio_trigger *trig)
> +{
> + struct max30210_state *st = iio_priv(indio_dev);
> +
> + if (st->trig != trig)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static int max30210_read_event(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + enum iio_event_info info, int *val, int *val2)
> +{
> + struct max30210_state *st = iio_priv(indio_dev);
> +
> + if (info == IIO_EV_INFO_VALUE) {
> + switch (dir) {
> + case IIO_EV_DIR_RISING:
> + switch (type) {
> + case IIO_EV_TYPE_THRESH:
> + return max30210_read_temp(st->regmap,
> + MAX30210_TEMP_ALM_HI_REG, val);
> + default:
> + return -EINVAL;
> + }
> + break;
break is unreachable and should be omitted.
> + case IIO_EV_DIR_FALLING:
> + switch (type) {
> + case IIO_EV_TYPE_THRESH:
> + return max30210_read_temp(st->regmap,
> + MAX30210_TEMP_ALM_LO_REG, val);
> + default:
> + return -EINVAL;
> + }
> + break;
Same.
> + default:
> + return -EINVAL;
> + }
> + }
> +
> + return -EINVAL;
Just return early after the if to avoid indenting the switch statement so much.
> +}
> +
> +static int max30210_write_event(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + enum iio_event_info info, int val, int val2)
> +{
> + struct max30210_state *st = iio_priv(indio_dev);
> +
> + if (info == IIO_EV_INFO_VALUE) {
> + switch (dir) {
> + case IIO_EV_DIR_RISING:
> + switch (type) {
> + case IIO_EV_TYPE_THRESH:
> + return max30210_write_temp(st->regmap,
> + MAX30210_TEMP_ALM_HI_REG, val);
> + default:
> + return -EINVAL;
> + }
> + break;
> + case IIO_EV_DIR_FALLING:
> + switch (type) {
> + case IIO_EV_TYPE_THRESH:
> + return max30210_write_temp(st->regmap,
> + MAX30210_TEMP_ALM_LO_REG, val);
> + default:
> + return -EINVAL;
> + }
> + break;
> + default:
> + return -EINVAL;
> + }
> + }
> +
> + return -EINVAL;
Same.
> +}
> +
> +static int max30210_read_event_config(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir)
> +{
> + struct max30210_state *st = iio_priv(indio_dev);
> + unsigned int val;
> + int ret;
> +
> + ret = regmap_read(st->regmap, MAX30210_INT_EN_REG, &val);
> + if (ret)
> + return ret;
> +
> + switch (dir) {
> + case IIO_EV_DIR_RISING:
> + switch (type) {
> + case IIO_EV_TYPE_THRESH:
> + return FIELD_GET(MAX30210_TEMP_HI_MASK, val);
> + default:
> + return -EINVAL;
> + }
> + break;
> + case IIO_EV_DIR_FALLING:
> + switch (type) {
> + case IIO_EV_TYPE_THRESH:
> + return FIELD_GET(MAX30210_TEMP_LO_MASK, val);
> + default:
> + return -EINVAL;
> + }
> + break;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int max30210_write_event_config(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir, int state)
> +{
> + struct max30210_state *st = iio_priv(indio_dev);
> + unsigned int val;
> +
> + switch (dir) {
> + case IIO_EV_DIR_RISING:
> + switch (type) {
> + case IIO_EV_TYPE_THRESH:
> + val = MAX30210_INT_EN(state, MAX30210_TEMP_HI_MASK);
> +
> + return regmap_update_bits(st->regmap,
> + MAX30210_INT_EN_REG,
> + MAX30210_TEMP_HI_MASK, val);
> + default:
> + return -EINVAL;
> + }
> + break;
> + case IIO_EV_DIR_FALLING:
> + switch (type) {
> + case IIO_EV_TYPE_THRESH:
> + val = MAX30210_INT_EN(state, MAX30210_TEMP_LO_MASK);
> +
> + return regmap_update_bits(st->regmap,
> + MAX30210_INT_EN_REG,
> + MAX30210_TEMP_LO_MASK, val);
> + default:
> + return -EINVAL;
> + }
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int max30210_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int *val,
> + int *val2, long mask)
> +{
> + struct max30210_state *st = iio_priv(indio_dev);
> + unsigned int uval;
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SCALE:
> + *val = 5;
> +
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + ret = regmap_read(st->regmap, MAX30210_TEMP_CONF_2_REG, &uval);
> + if (ret)
> + return ret;
> +
> + uval = FIELD_GET(MAX30210_TEMP_PERIOD_MASK, uval);
> +
> + *val = 8;
> +
> + /**
> + * register values 0x9 or above have the same sample
> + * rate of 8Hz
> + */
> + *val2 = uval >= 0x9 ? 1 : BIT(0x9 - uval);
> +
> + return IIO_VAL_FRACTIONAL;
> + case IIO_CHAN_INFO_RAW:
> + ret = iio_device_claim_direct_mode(indio_dev);
If the buffer is enabled, can we just skip setting the converion mode
and read the TEMP_DATA register to get the last value measured?
Also, iio_device_claim_direct_mode() doesn't exist anymore, so I wonder
which kernel version was used to compile and test this patch.
We now have IIO_DEV_ACQUIRE_DIRECT_MODE() that can be used in cases
like this to avoid the goto.
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(st->regmap, MAX30210_TEMP_CONV_REG,
> + MAX30210_CONV_T_MASK);
> + if (ret)
> + goto release_dmode;
> +
> + fsleep(8000);
> +
> + ret = max30210_read_temp(st->regmap, MAX30210_TEMP_DATA_REG, val);
> +
> +release_dmode:
> + iio_device_release_direct_mode(indio_dev);
> + return ret;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int max30210_read_avail(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + const int **vals, int *type, int *length,
> + long mask)
> +{
> + switch (mask) {
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + *vals = samp_freq_avail;
> + *type = IIO_VAL_INT_PLUS_MICRO;
> + *length = ARRAY_SIZE(samp_freq_avail);
> +
> + return IIO_AVAIL_LIST;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int max30210_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int val,
> + int val2, long mask)
> +{
> + struct max30210_state *st = iio_priv(indio_dev);
> + u64 data;
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + ret = iio_device_claim_direct_mode(indio_dev);
> + if (ret)
> + return ret;
> +
> + /**
> + * micro_value = val * 1000000 + val2
> + * reg_value = ((micro_value * 64) / 1000000) - 1
> + */
> + data = (val * MICRO + val2) << 6;
This will end up with wierd resutls if val or val2 is negative.
Can we write << 6 as * 64 to match the comment? The compiler is smart enough
to make the optimization for us.
> + do_div(data, MICRO);
> +
> + data = fls_long(data - 1);
> + data = FIELD_PREP(MAX30210_TEMP_PERIOD_MASK, data);
> +
> + ret = regmap_update_bits(st->regmap, MAX30210_TEMP_CONF_2_REG,
> + MAX30210_TEMP_PERIOD_MASK,
> + (unsigned int)data);
> +
> + iio_device_release_direct_mode(indio_dev);
> + return ret;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int max30210_write_raw_get_fmt(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + long mask)
> +{
> + switch (mask) {
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + return IIO_VAL_INT_PLUS_MICRO;
This is the default, so we should not need this function at all.
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static const struct iio_trigger_ops max30210_trigger_ops = {
> + .validate_device = &iio_trigger_validate_own_device,
> +};
> +
> +static int max30210_set_watermark(struct iio_dev *indio_dev, unsigned int val)
> +{
> + struct max30210_state *st = iio_priv(indio_dev);
> + unsigned int reg;
> + int ret;
> +
> + if (val < 1 || val > MAX30210_FIFO_SIZE)
> + return -EINVAL;
> +
> + reg = MAX30210_FIFO_SIZE - val;
> +
> + ret = regmap_write(st->regmap, MAX30210_FIFO_CONF_1_REG, reg);
> + if (ret)
> + return ret;
> +
> + st->watermark = val;
> +
> + return 0;
> +}
> +
> +static ssize_t hwfifo_watermark_show(struct device *dev,
> + struct device_attribute *devattr,
> + char *buf)
> +{
> + struct max30210_state *st = iio_priv(dev_to_iio_dev(dev));
> +
> + return sysfs_emit(buf, "%d\n", st->watermark);
> +}
> +
> +IIO_STATIC_CONST_DEVICE_ATTR(hwfifo_watermark_min, "1");
> +IIO_STATIC_CONST_DEVICE_ATTR(hwfifo_watermark_max,
> + __stringify(MAX30210_FIFO_SIZE));
> +static IIO_DEVICE_ATTR_RO(hwfifo_watermark, 0);
> +
> +static const struct iio_dev_attr *max30210_fifo_attributes[] = {
> + &iio_dev_attr_hwfifo_watermark_min,
> + &iio_dev_attr_hwfifo_watermark_max,
> + &iio_dev_attr_hwfifo_watermark,
> + NULL,
> +};
> +
> +static int max30210_buffer_preenable(struct iio_dev *indio_dev)
> +{
> + struct max30210_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + ret = regmap_update_bits(st->regmap, MAX30210_INT_EN_REG,
> + MAX30210_A_FULL_MASK, MAX30210_A_FULL_MASK);
> + if (ret)
> + return ret;
> +
> + ret = regmap_update_bits(st->regmap, MAX30210_FIFO_CONF_2_REG,
> + MAX30210_FLUSH_FIFO_MASK,
> + MAX30210_FLUSH_FIFO_MASK);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(st->regmap, MAX30210_TEMP_CONV_REG,
> + MAX30210_AUTO_MASK | MAX30210_CONV_T_MASK);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +static int max30210_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> + struct max30210_state *st = iio_priv(indio_dev);
> + int ret;
> +
Unwinding should be the revese order of the setup. If there is a reason
for it to be in a different order, it needs a comment to explain.
> + ret = regmap_update_bits(st->regmap, MAX30210_INT_EN_REG,
> + MAX30210_A_FULL_MASK, 0x0);
> + if (ret)
> + return ret;
> +
> + ret = regmap_update_bits(st->regmap, MAX30210_FIFO_CONF_2_REG,
> + MAX30210_FLUSH_FIFO_MASK,
> + MAX30210_FLUSH_FIFO_MASK);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(st->regmap, MAX30210_TEMP_CONV_REG, 0x0);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +static const struct iio_buffer_setup_ops max30210_buffer_ops = {
> + .preenable = max30210_buffer_preenable,
> + .postdisable = max30210_buffer_postdisable,
> +};
> +
> +static const struct iio_info max30210_info = {
> + .read_raw = max30210_read_raw,
> + .read_avail = max30210_read_avail,
> + .write_raw = max30210_write_raw,
> + .write_raw_get_fmt = max30210_write_raw_get_fmt,
> + .hwfifo_set_watermark = max30210_set_watermark,
> + .debugfs_reg_access = &max30210_reg_access,
> + .validate_trigger = &max30210_validate_trigger,
Can we just use iio_validate_own_trigger()?
> + .read_event_value = max30210_read_event,
> + .write_event_value = max30210_write_event,
> + .write_event_config = max30210_write_event_config,
> + .read_event_config = max30210_read_event_config,
> +};
> +
> +static const struct iio_event_spec max30210_events[] = {
> + {
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_RISING,
> + .mask_separate = BIT(IIO_EV_INFO_VALUE) | BIT(IIO_EV_INFO_ENABLE),
> + }, {
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_FALLING,
> + .mask_separate = BIT(IIO_EV_INFO_VALUE) | BIT(IIO_EV_INFO_ENABLE),
> + },
> +};
> +
> +static const struct iio_chan_spec max30210_channels = {
> + .type = IIO_TEMP,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE) |
> + BIT(IIO_CHAN_INFO_SAMP_FREQ),
> + .info_mask_separate_available = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> + .output = 0,
> + .scan_index = 0,
> + .event_spec = max30210_events,
> + .num_event_specs = ARRAY_SIZE(max30210_events),
> + .scan_type = {
> + .sign = 's',
> + .realbits = 16,
> + .storagebits = 32,
> + .shift = 8,
> + .endianness = IIO_BE,
> + },
> +};
> +
> +static int max30210_setup(struct max30210_state *st, struct device *dev)
> +{
> + unsigned int val;
> +
> + /* Power down to reset device */
> + st->powerdown_gpio = devm_gpiod_get_optional(dev, "powerdown",
> + GPIOD_OUT_HIGH);
> + if (IS_ERR(st->powerdown_gpio))
> + return dev_err_probe(dev, PTR_ERR(st->powerdown_gpio),
> + "Failed to request powerdown GPIO.\n");
> +
> + /* Power up device */
> + gpiod_set_value(st->powerdown_gpio, 0);
Usually, if the gpio is not wired up, we would reset the device via
a software reset. It looks like this has one in the SYSTEM_CONFIGURATION
register.
> +
> + fsleep(700);
> +
> + /* Clear status byte */
> + return regmap_read(st->regmap, MAX30210_STATUS_REG, &val);
> +}
> +
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 2/2] iio: temperature: add ADI MAX30210 driver
2026-02-26 16:30 ` [PATCH 2/2] iio: temperature: add ADI MAX30210 driver John Erasmus Mari Geronimo
2026-02-26 17:48 ` Andy Shevchenko
2026-02-26 20:08 ` David Lechner
@ 2026-02-26 21:26 ` kernel test robot
2026-02-26 22:29 ` kernel test robot
2026-02-28 13:05 ` Jonathan Cameron
4 siblings, 0 replies; 11+ messages in thread
From: kernel test robot @ 2026-02-26 21:26 UTC (permalink / raw)
To: John Erasmus Mari Geronimo, linux-iio
Cc: oe-kbuild-all, devicetree, linux-kernel, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko
Hi John,
kernel test robot noticed the following build errors:
[auto build test ERROR on jic23-iio/togreg]
[also build test ERROR on linus/master v7.0-rc1 next-20260226]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/John-Erasmus-Mari-Geronimo/dt-bindings-iio-temperature-add-ADI-MAX30210/20260227-013306
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link: https://lore.kernel.org/r/20260226163041.169786-3-johnerasmusmari.geronimo%40analog.com
patch subject: [PATCH 2/2] iio: temperature: add ADI MAX30210 driver
config: nios2-allmodconfig (https://download.01.org/0day-ci/archive/20260227/202602270554.gpbYaUtd-lkp@intel.com/config)
compiler: nios2-linux-gcc (GCC) 11.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260227/202602270554.gpbYaUtd-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202602270554.gpbYaUtd-lkp@intel.com/
All errors (new ones prefixed by >>):
drivers/iio/temperature/max30210.c: In function 'max30210_read_raw':
>> drivers/iio/temperature/max30210.c:412:23: error: implicit declaration of function 'iio_device_claim_direct_mode'; did you mean 'iio_device_claim_direct'? [-Werror=implicit-function-declaration]
412 | ret = iio_device_claim_direct_mode(indio_dev);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
| iio_device_claim_direct
>> drivers/iio/temperature/max30210.c:426:17: error: implicit declaration of function 'iio_device_release_direct_mode'; did you mean 'iio_device_release_direct'? [-Werror=implicit-function-declaration]
426 | iio_device_release_direct_mode(indio_dev);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
| iio_device_release_direct
drivers/iio/temperature/max30210.c: At top level:
>> drivers/iio/temperature/max30210.c:604:31: error: initialization of 'int (*)(struct iio_dev *, const struct iio_chan_spec *, enum iio_event_type, enum iio_event_direction, bool)' {aka 'int (*)(struct iio_dev *, const struct iio_chan_spec *, enum iio_event_type, enum iio_event_direction, _Bool)'} from incompatible pointer type 'int (*)(struct iio_dev *, const struct iio_chan_spec *, enum iio_event_type, enum iio_event_direction, int)' [-Werror=incompatible-pointer-types]
604 | .write_event_config = max30210_write_event_config,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/iio/temperature/max30210.c:604:31: note: (near initialization for 'max30210_info.write_event_config')
cc1: some warnings being treated as errors
vim +412 drivers/iio/temperature/max30210.c
381
382 static int max30210_read_raw(struct iio_dev *indio_dev,
383 struct iio_chan_spec const *chan, int *val,
384 int *val2, long mask)
385 {
386 struct max30210_state *st = iio_priv(indio_dev);
387 unsigned int uval;
388 int ret;
389
390 switch (mask) {
391 case IIO_CHAN_INFO_SCALE:
392 *val = 5;
393
394 return IIO_VAL_INT;
395 case IIO_CHAN_INFO_SAMP_FREQ:
396 ret = regmap_read(st->regmap, MAX30210_TEMP_CONF_2_REG, &uval);
397 if (ret)
398 return ret;
399
400 uval = FIELD_GET(MAX30210_TEMP_PERIOD_MASK, uval);
401
402 *val = 8;
403
404 /**
405 * register values 0x9 or above have the same sample
406 * rate of 8Hz
407 */
408 *val2 = uval >= 0x9 ? 1 : BIT(0x9 - uval);
409
410 return IIO_VAL_FRACTIONAL;
411 case IIO_CHAN_INFO_RAW:
> 412 ret = iio_device_claim_direct_mode(indio_dev);
413 if (ret)
414 return ret;
415
416 ret = regmap_write(st->regmap, MAX30210_TEMP_CONV_REG,
417 MAX30210_CONV_T_MASK);
418 if (ret)
419 goto release_dmode;
420
421 fsleep(8000);
422
423 ret = max30210_read_temp(st->regmap, MAX30210_TEMP_DATA_REG, val);
424
425 release_dmode:
> 426 iio_device_release_direct_mode(indio_dev);
427 return ret;
428 default:
429 return -EINVAL;
430 }
431 }
432
433 static int max30210_read_avail(struct iio_dev *indio_dev,
434 struct iio_chan_spec const *chan,
435 const int **vals, int *type, int *length,
436 long mask)
437 {
438 switch (mask) {
439 case IIO_CHAN_INFO_SAMP_FREQ:
440 *vals = samp_freq_avail;
441 *type = IIO_VAL_INT_PLUS_MICRO;
442 *length = ARRAY_SIZE(samp_freq_avail);
443
444 return IIO_AVAIL_LIST;
445 default:
446 return -EINVAL;
447 }
448 }
449
450 static int max30210_write_raw(struct iio_dev *indio_dev,
451 struct iio_chan_spec const *chan, int val,
452 int val2, long mask)
453 {
454 struct max30210_state *st = iio_priv(indio_dev);
455 u64 data;
456 int ret;
457
458 switch (mask) {
459 case IIO_CHAN_INFO_SAMP_FREQ:
460 ret = iio_device_claim_direct_mode(indio_dev);
461 if (ret)
462 return ret;
463
464 /**
465 * micro_value = val * 1000000 + val2
466 * reg_value = ((micro_value * 64) / 1000000) - 1
467 */
468 data = (val * MICRO + val2) << 6;
469 do_div(data, MICRO);
470
471 data = fls_long(data - 1);
472 data = FIELD_PREP(MAX30210_TEMP_PERIOD_MASK, data);
473
474 ret = regmap_update_bits(st->regmap, MAX30210_TEMP_CONF_2_REG,
475 MAX30210_TEMP_PERIOD_MASK,
476 (unsigned int)data);
477
478 iio_device_release_direct_mode(indio_dev);
479 return ret;
480 default:
481 return -EINVAL;
482 }
483 }
484
485 static int max30210_write_raw_get_fmt(struct iio_dev *indio_dev,
486 struct iio_chan_spec const *chan,
487 long mask)
488 {
489 switch (mask) {
490 case IIO_CHAN_INFO_SAMP_FREQ:
491 return IIO_VAL_INT_PLUS_MICRO;
492 default:
493 return -EINVAL;
494 }
495 }
496
497 static const struct iio_trigger_ops max30210_trigger_ops = {
498 .validate_device = &iio_trigger_validate_own_device,
499 };
500
501 static int max30210_set_watermark(struct iio_dev *indio_dev, unsigned int val)
502 {
503 struct max30210_state *st = iio_priv(indio_dev);
504 unsigned int reg;
505 int ret;
506
507 if (val < 1 || val > MAX30210_FIFO_SIZE)
508 return -EINVAL;
509
510 reg = MAX30210_FIFO_SIZE - val;
511
512 ret = regmap_write(st->regmap, MAX30210_FIFO_CONF_1_REG, reg);
513 if (ret)
514 return ret;
515
516 st->watermark = val;
517
518 return 0;
519 }
520
521 static ssize_t hwfifo_watermark_show(struct device *dev,
522 struct device_attribute *devattr,
523 char *buf)
524 {
525 struct max30210_state *st = iio_priv(dev_to_iio_dev(dev));
526
527 return sysfs_emit(buf, "%d\n", st->watermark);
528 }
529
530 IIO_STATIC_CONST_DEVICE_ATTR(hwfifo_watermark_min, "1");
531 IIO_STATIC_CONST_DEVICE_ATTR(hwfifo_watermark_max,
532 __stringify(MAX30210_FIFO_SIZE));
533 static IIO_DEVICE_ATTR_RO(hwfifo_watermark, 0);
534
535 static const struct iio_dev_attr *max30210_fifo_attributes[] = {
536 &iio_dev_attr_hwfifo_watermark_min,
537 &iio_dev_attr_hwfifo_watermark_max,
538 &iio_dev_attr_hwfifo_watermark,
539 NULL,
540 };
541
542 static int max30210_buffer_preenable(struct iio_dev *indio_dev)
543 {
544 struct max30210_state *st = iio_priv(indio_dev);
545 int ret;
546
547 ret = regmap_update_bits(st->regmap, MAX30210_INT_EN_REG,
548 MAX30210_A_FULL_MASK, MAX30210_A_FULL_MASK);
549 if (ret)
550 return ret;
551
552 ret = regmap_update_bits(st->regmap, MAX30210_FIFO_CONF_2_REG,
553 MAX30210_FLUSH_FIFO_MASK,
554 MAX30210_FLUSH_FIFO_MASK);
555 if (ret)
556 return ret;
557
558 ret = regmap_write(st->regmap, MAX30210_TEMP_CONV_REG,
559 MAX30210_AUTO_MASK | MAX30210_CONV_T_MASK);
560 if (ret)
561 return ret;
562
563 return 0;
564 }
565
566 static int max30210_buffer_postdisable(struct iio_dev *indio_dev)
567 {
568 struct max30210_state *st = iio_priv(indio_dev);
569 int ret;
570
571 ret = regmap_update_bits(st->regmap, MAX30210_INT_EN_REG,
572 MAX30210_A_FULL_MASK, 0x0);
573 if (ret)
574 return ret;
575
576 ret = regmap_update_bits(st->regmap, MAX30210_FIFO_CONF_2_REG,
577 MAX30210_FLUSH_FIFO_MASK,
578 MAX30210_FLUSH_FIFO_MASK);
579 if (ret)
580 return ret;
581
582 ret = regmap_write(st->regmap, MAX30210_TEMP_CONV_REG, 0x0);
583 if (ret)
584 return ret;
585
586 return 0;
587 }
588
589 static const struct iio_buffer_setup_ops max30210_buffer_ops = {
590 .preenable = max30210_buffer_preenable,
591 .postdisable = max30210_buffer_postdisable,
592 };
593
594 static const struct iio_info max30210_info = {
595 .read_raw = max30210_read_raw,
596 .read_avail = max30210_read_avail,
597 .write_raw = max30210_write_raw,
598 .write_raw_get_fmt = max30210_write_raw_get_fmt,
599 .hwfifo_set_watermark = max30210_set_watermark,
600 .debugfs_reg_access = &max30210_reg_access,
601 .validate_trigger = &max30210_validate_trigger,
602 .read_event_value = max30210_read_event,
603 .write_event_value = max30210_write_event,
> 604 .write_event_config = max30210_write_event_config,
605 .read_event_config = max30210_read_event_config,
606 };
607
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 2/2] iio: temperature: add ADI MAX30210 driver
2026-02-26 16:30 ` [PATCH 2/2] iio: temperature: add ADI MAX30210 driver John Erasmus Mari Geronimo
` (2 preceding siblings ...)
2026-02-26 21:26 ` kernel test robot
@ 2026-02-26 22:29 ` kernel test robot
2026-02-28 13:05 ` Jonathan Cameron
4 siblings, 0 replies; 11+ messages in thread
From: kernel test robot @ 2026-02-26 22:29 UTC (permalink / raw)
To: John Erasmus Mari Geronimo, linux-iio
Cc: llvm, oe-kbuild-all, devicetree, linux-kernel, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko
Hi John,
kernel test robot noticed the following build errors:
[auto build test ERROR on jic23-iio/togreg]
[also build test ERROR on linus/master v7.0-rc1 next-20260226]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/John-Erasmus-Mari-Geronimo/dt-bindings-iio-temperature-add-ADI-MAX30210/20260227-013306
base: https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link: https://lore.kernel.org/r/20260226163041.169786-3-johnerasmusmari.geronimo%40analog.com
patch subject: [PATCH 2/2] iio: temperature: add ADI MAX30210 driver
config: sparc64-allmodconfig (https://download.01.org/0day-ci/archive/20260227/202602270610.Batqc2is-lkp@intel.com/config)
compiler: clang version 23.0.0git (https://github.com/llvm/llvm-project 9a109fbb6e184ec9bcce10615949f598f4c974a9)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260227/202602270610.Batqc2is-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202602270610.Batqc2is-lkp@intel.com/
All errors (new ones prefixed by >>):
>> drivers/iio/temperature/max30210.c:412:9: error: call to undeclared function 'iio_device_claim_direct_mode'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
412 | ret = iio_device_claim_direct_mode(indio_dev);
| ^
drivers/iio/temperature/max30210.c:412:9: note: did you mean 'iio_device_claim_direct'?
include/linux/iio/iio.h:687:20: note: 'iio_device_claim_direct' declared here
687 | static inline bool iio_device_claim_direct(struct iio_dev *indio_dev)
| ^
>> drivers/iio/temperature/max30210.c:426:3: error: call to undeclared function 'iio_device_release_direct_mode'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
426 | iio_device_release_direct_mode(indio_dev);
| ^
drivers/iio/temperature/max30210.c:460:9: error: call to undeclared function 'iio_device_claim_direct_mode'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
460 | ret = iio_device_claim_direct_mode(indio_dev);
| ^
drivers/iio/temperature/max30210.c:478:3: error: call to undeclared function 'iio_device_release_direct_mode'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
478 | iio_device_release_direct_mode(indio_dev);
| ^
>> drivers/iio/temperature/max30210.c:604:24: error: incompatible function pointer types initializing 'int (*)(struct iio_dev *, const struct iio_chan_spec *, enum iio_event_type, enum iio_event_direction, bool)' (aka 'int (*)(struct iio_dev *, const struct iio_chan_spec *, enum iio_event_type, enum iio_event_direction, _Bool)') with an expression of type 'int (struct iio_dev *, const struct iio_chan_spec *, enum iio_event_type, enum iio_event_direction, int)' [-Wincompatible-function-pointer-types]
604 | .write_event_config = max30210_write_event_config,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~
5 errors generated.
vim +/iio_device_claim_direct_mode +412 drivers/iio/temperature/max30210.c
381
382 static int max30210_read_raw(struct iio_dev *indio_dev,
383 struct iio_chan_spec const *chan, int *val,
384 int *val2, long mask)
385 {
386 struct max30210_state *st = iio_priv(indio_dev);
387 unsigned int uval;
388 int ret;
389
390 switch (mask) {
391 case IIO_CHAN_INFO_SCALE:
392 *val = 5;
393
394 return IIO_VAL_INT;
395 case IIO_CHAN_INFO_SAMP_FREQ:
396 ret = regmap_read(st->regmap, MAX30210_TEMP_CONF_2_REG, &uval);
397 if (ret)
398 return ret;
399
400 uval = FIELD_GET(MAX30210_TEMP_PERIOD_MASK, uval);
401
402 *val = 8;
403
404 /**
405 * register values 0x9 or above have the same sample
406 * rate of 8Hz
407 */
408 *val2 = uval >= 0x9 ? 1 : BIT(0x9 - uval);
409
410 return IIO_VAL_FRACTIONAL;
411 case IIO_CHAN_INFO_RAW:
> 412 ret = iio_device_claim_direct_mode(indio_dev);
413 if (ret)
414 return ret;
415
416 ret = regmap_write(st->regmap, MAX30210_TEMP_CONV_REG,
417 MAX30210_CONV_T_MASK);
418 if (ret)
419 goto release_dmode;
420
421 fsleep(8000);
422
423 ret = max30210_read_temp(st->regmap, MAX30210_TEMP_DATA_REG, val);
424
425 release_dmode:
> 426 iio_device_release_direct_mode(indio_dev);
427 return ret;
428 default:
429 return -EINVAL;
430 }
431 }
432
433 static int max30210_read_avail(struct iio_dev *indio_dev,
434 struct iio_chan_spec const *chan,
435 const int **vals, int *type, int *length,
436 long mask)
437 {
438 switch (mask) {
439 case IIO_CHAN_INFO_SAMP_FREQ:
440 *vals = samp_freq_avail;
441 *type = IIO_VAL_INT_PLUS_MICRO;
442 *length = ARRAY_SIZE(samp_freq_avail);
443
444 return IIO_AVAIL_LIST;
445 default:
446 return -EINVAL;
447 }
448 }
449
450 static int max30210_write_raw(struct iio_dev *indio_dev,
451 struct iio_chan_spec const *chan, int val,
452 int val2, long mask)
453 {
454 struct max30210_state *st = iio_priv(indio_dev);
455 u64 data;
456 int ret;
457
458 switch (mask) {
459 case IIO_CHAN_INFO_SAMP_FREQ:
460 ret = iio_device_claim_direct_mode(indio_dev);
461 if (ret)
462 return ret;
463
464 /**
465 * micro_value = val * 1000000 + val2
466 * reg_value = ((micro_value * 64) / 1000000) - 1
467 */
468 data = (val * MICRO + val2) << 6;
469 do_div(data, MICRO);
470
471 data = fls_long(data - 1);
472 data = FIELD_PREP(MAX30210_TEMP_PERIOD_MASK, data);
473
474 ret = regmap_update_bits(st->regmap, MAX30210_TEMP_CONF_2_REG,
475 MAX30210_TEMP_PERIOD_MASK,
476 (unsigned int)data);
477
478 iio_device_release_direct_mode(indio_dev);
479 return ret;
480 default:
481 return -EINVAL;
482 }
483 }
484
485 static int max30210_write_raw_get_fmt(struct iio_dev *indio_dev,
486 struct iio_chan_spec const *chan,
487 long mask)
488 {
489 switch (mask) {
490 case IIO_CHAN_INFO_SAMP_FREQ:
491 return IIO_VAL_INT_PLUS_MICRO;
492 default:
493 return -EINVAL;
494 }
495 }
496
497 static const struct iio_trigger_ops max30210_trigger_ops = {
498 .validate_device = &iio_trigger_validate_own_device,
499 };
500
501 static int max30210_set_watermark(struct iio_dev *indio_dev, unsigned int val)
502 {
503 struct max30210_state *st = iio_priv(indio_dev);
504 unsigned int reg;
505 int ret;
506
507 if (val < 1 || val > MAX30210_FIFO_SIZE)
508 return -EINVAL;
509
510 reg = MAX30210_FIFO_SIZE - val;
511
512 ret = regmap_write(st->regmap, MAX30210_FIFO_CONF_1_REG, reg);
513 if (ret)
514 return ret;
515
516 st->watermark = val;
517
518 return 0;
519 }
520
521 static ssize_t hwfifo_watermark_show(struct device *dev,
522 struct device_attribute *devattr,
523 char *buf)
524 {
525 struct max30210_state *st = iio_priv(dev_to_iio_dev(dev));
526
527 return sysfs_emit(buf, "%d\n", st->watermark);
528 }
529
530 IIO_STATIC_CONST_DEVICE_ATTR(hwfifo_watermark_min, "1");
531 IIO_STATIC_CONST_DEVICE_ATTR(hwfifo_watermark_max,
532 __stringify(MAX30210_FIFO_SIZE));
533 static IIO_DEVICE_ATTR_RO(hwfifo_watermark, 0);
534
535 static const struct iio_dev_attr *max30210_fifo_attributes[] = {
536 &iio_dev_attr_hwfifo_watermark_min,
537 &iio_dev_attr_hwfifo_watermark_max,
538 &iio_dev_attr_hwfifo_watermark,
539 NULL,
540 };
541
542 static int max30210_buffer_preenable(struct iio_dev *indio_dev)
543 {
544 struct max30210_state *st = iio_priv(indio_dev);
545 int ret;
546
547 ret = regmap_update_bits(st->regmap, MAX30210_INT_EN_REG,
548 MAX30210_A_FULL_MASK, MAX30210_A_FULL_MASK);
549 if (ret)
550 return ret;
551
552 ret = regmap_update_bits(st->regmap, MAX30210_FIFO_CONF_2_REG,
553 MAX30210_FLUSH_FIFO_MASK,
554 MAX30210_FLUSH_FIFO_MASK);
555 if (ret)
556 return ret;
557
558 ret = regmap_write(st->regmap, MAX30210_TEMP_CONV_REG,
559 MAX30210_AUTO_MASK | MAX30210_CONV_T_MASK);
560 if (ret)
561 return ret;
562
563 return 0;
564 }
565
566 static int max30210_buffer_postdisable(struct iio_dev *indio_dev)
567 {
568 struct max30210_state *st = iio_priv(indio_dev);
569 int ret;
570
571 ret = regmap_update_bits(st->regmap, MAX30210_INT_EN_REG,
572 MAX30210_A_FULL_MASK, 0x0);
573 if (ret)
574 return ret;
575
576 ret = regmap_update_bits(st->regmap, MAX30210_FIFO_CONF_2_REG,
577 MAX30210_FLUSH_FIFO_MASK,
578 MAX30210_FLUSH_FIFO_MASK);
579 if (ret)
580 return ret;
581
582 ret = regmap_write(st->regmap, MAX30210_TEMP_CONV_REG, 0x0);
583 if (ret)
584 return ret;
585
586 return 0;
587 }
588
589 static const struct iio_buffer_setup_ops max30210_buffer_ops = {
590 .preenable = max30210_buffer_preenable,
591 .postdisable = max30210_buffer_postdisable,
592 };
593
594 static const struct iio_info max30210_info = {
595 .read_raw = max30210_read_raw,
596 .read_avail = max30210_read_avail,
597 .write_raw = max30210_write_raw,
598 .write_raw_get_fmt = max30210_write_raw_get_fmt,
599 .hwfifo_set_watermark = max30210_set_watermark,
600 .debugfs_reg_access = &max30210_reg_access,
601 .validate_trigger = &max30210_validate_trigger,
602 .read_event_value = max30210_read_event,
603 .write_event_value = max30210_write_event,
> 604 .write_event_config = max30210_write_event_config,
605 .read_event_config = max30210_read_event_config,
606 };
607
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 2/2] iio: temperature: add ADI MAX30210 driver
2026-02-26 16:30 ` [PATCH 2/2] iio: temperature: add ADI MAX30210 driver John Erasmus Mari Geronimo
` (3 preceding siblings ...)
2026-02-26 22:29 ` kernel test robot
@ 2026-02-28 13:05 ` Jonathan Cameron
4 siblings, 0 replies; 11+ messages in thread
From: Jonathan Cameron @ 2026-02-28 13:05 UTC (permalink / raw)
To: John Erasmus Mari Geronimo
Cc: linux-iio, devicetree, linux-kernel, David Lechner, Nuno Sá,
Andy Shevchenko
On Fri, 27 Feb 2026 00:30:41 +0800
John Erasmus Mari Geronimo <johnerasmusmari.geronimo@analog.com> wrote:
> MAX30210 ±0.1°C Accurate Ultra-Small Low-Power Digital Temperature Sensor
For a temperature sensor, this description doesn't give enough detail
on why you are proposing an IIO driver rather than a hwmon one.
Please add more for v2. Focus on what the part is and features you are supporting
that don't have a path to being supporting in hwmon. E.g. the fifo.
The other thing to add a brief note on is why this support cannot be easily
added to an existing driver.
>
> Signed-off-by: John Erasmus Mari Geronimo <johnerasmusmari.geronimo@analog.com>
Various additional comments inline from me.
Welcome to IIO!
Jonathan
> diff --git a/drivers/iio/temperature/max30210.c b/drivers/iio/temperature/max30210.c
> new file mode 100644
> index 000000000000..aaa3a26be131
> --- /dev/null
> +++ b/drivers/iio/temperature/max30210.c
> @@ -0,0 +1,758 @@
> +
> +#define MAX30210_STATUS_REG 0x00
> +#define MAX30210_INT_EN_REG 0x02
> +#define MAX30210_FIFO_DATA_REG 0x08
> +#define MAX30210_FIFO_CONF_1_REG 0x09
> +#define MAX30210_FIFO_CONF_2_REG 0x0A
> +#define MAX30210_SYS_CONF_REG 0x11
> +#define MAX30210_PIN_CONF_REG 0x12
> +#define MAX30210_TEMP_ALM_HI_REG 0x22
> +#define MAX30210_TEMP_ALM_LO_REG 0x24
> +#define MAX30210_TEMP_INC_THRESH_REG 0x26
> +#define MAX30210_TEMP_DEC_THRESH_REG 0x27
> +#define MAX30210_TEMP_CONF_1_REG 0x28
> +#define MAX30210_TEMP_CONF_2_REG 0x29
> +#define MAX30210_TEMP_CONV_REG 0x2A
> +#define MAX30210_TEMP_DATA_REG 0x2B
> +#define MAX30210_TEMP_SLOPE_REG 0x2D
> +#define MAX30210_UNIQUE_ID_REG 0x30
> +#define MAX30210_PART_ID_REG 0xFF
> +
> +#define MAX30210_A_FULL_MASK BIT(7)
I'm very keen that field names reflect which register they are in.
That makes it much easier to review whether they are being correctly
used. Usual way to do that is have a prefix that incorporates enough
of the register name to work out that mapping.
> +#define MAX30210_TEMP_RDY_MASK BIT(6)
> +#define MAX30210_TEMP_DEC_MASK BIT(5)
> +#define MAX30210_TEMP_INC_MASK BIT(4)
> +#define MAX30210_TEMP_LO_MASK BIT(3)
> +#define MAX30210_TEMP_HI_MASK BIT(2)
> +#define MAX30210_PWR_RDY_MASK BIT(0)
> +
> +#define MAX30210_FLUSH_FIFO_MASK BIT(4)
> +
> +#define MAX30210_EXT_CNV_EN_MASK BIT(7)
> +#define MAX30210_EXT_CVT_ICFG_MASK BIT(6)
> +#define MAX30210_INT_FCFG_MASK GENMASK(3, 2)
> +#define MAX30210_INT_OCFG_MASK GENMASK(1, 0)
> +
> +#define MAX30210_CHG_DET_EN_MASK BIT(3)
> +#define MAX30210_RATE_CHG_FILTER_MASK GENMASK(2, 0)
> +
> +#define MAX30210_TEMP_PERIOD_MASK GENMASK(3, 0)
> +#define MAX30210_ALERT_MODE_MASK BIT(7)
> +
> +#define MAX30210_AUTO_MASK BIT(1)
> +#define MAX30210_CONV_T_MASK BIT(0)
> +
> +#define MAX30210_PART_ID 0x45
> +#define MAX30210_FIFO_SIZE 64
> +#define MAX30210_FIFO_INVAL_DATA GENMASK(23, 0)
> +#define MAX30210_WATERMARK_DEFAULT (0x40 - 0x1F)
> +
> +#define MAX30210_INT_EN(state, mask) ((state) ? (mask) : 0x0)
Use regmap_assign_bits() to replace this macro.
> +
> +struct max30210_state {
> + /*
> + * Prevent simultaneous access to the i2c client.
Why does that matter? I'd imagine you have some read / modify write
sequences or need to not change the mode whilst something else is going
on?
> + */
> + struct mutex lock;
> + struct regmap *regmap;
> + struct iio_trigger *trig;
> + struct gpio_desc *powerdown_gpio;
> + u8 watermark;
> + u8 data[3 * MAX30210_FIFO_SIZE] __aligned(IIO_DMA_MINALIGN);
> +};
> +static int max30210_read_temp(struct regmap *regmap, unsigned int reg,
> + int *temp)
> +{
> + u8 uval[2] __aligned(IIO_DMA_MINALIGN);
As below, forcing alignment on the stack doesn't work for this purpose.
Needs to be on the heap. Put it next to data in the _state structure.
However, this is an i2c driver. I2C doesn't have any such requirements
on buffer alignment because it always bounce buffers data if needed.
> + int ret;
> +
> + ret = regmap_bulk_read(regmap, reg, uval, 2);
> + if (ret)
> + return ret;
> +
> + *temp = sign_extend32(get_unaligned_be16(uval), 15);
> +
> + return IIO_VAL_INT;
> +}
> +
> +static int max30210_write_temp(struct regmap *regmap, unsigned int reg,
> + int temp)
> +{
> + u8 uval[2] __aligned(IIO_DMA_MINALIGN);
You've miss understood the purpose of IIO_DMA_MINALIGN.
Make sure to look into that but the short answer is you can't do it on the stack.
> +
> + put_unaligned_be16(temp, uval);
> +
> + return regmap_bulk_write(regmap, reg, uval, 2);
Use a __be16 type for the buffer and then sizeof() for the 2
> +}
> +
> +static void max30210_fifo_read(struct iio_dev *indio_dev)
> +{
> + struct max30210_state *st = iio_priv(indio_dev);
> + u32 samp;
> + int ret, i, j;
> +
> + ret = regmap_bulk_read(st->regmap, MAX30210_FIFO_DATA_REG,
> + st->data, 3 * st->watermark);
> + if (ret < 0)
> + return dev_err(&indio_dev->dev, "Failed to read from fifo.\n");
> +
> + for (i = 0; i < st->watermark; i++) {
u32 samp = 0;
> + samp = 0;
> +
> + for (j = 0; j < 3; j++) {
> + samp <<= 8;
> + samp |= st->data[3 * i + j];
Looks like get_unaligned_be24() or similar. Use that instead of opencoding
the endian conversion. However, you are claiming this is a big endian channel
and this is an endian conversion so something is wrong. If you keep it as
a big endian channel, this probably wants to be simply memcpy()
> + }
> +
> + if (samp == MAX30210_FIFO_INVAL_DATA) {
> + dev_err(&indio_dev->dev, "Invalid data\n");
> + continue;
> + }
> +
> + iio_push_to_buffers(indio_dev, &samp);
> + }
> +}
> +
> +static irqreturn_t max30210_trigger_handler(int irq, void *p)
> +{
> + struct iio_poll_func *pf = p;
> + struct iio_dev *indio_dev = pf->indio_dev;
> + struct max30210_state *st = iio_priv(indio_dev);
> + unsigned int status;
> + int ret;
> +
> + guard(mutex)(&st->lock);
> +
> + ret = regmap_read(st->regmap, MAX30210_STATUS_REG, &status);
> + if (ret) {
> + dev_err(&indio_dev->dev, "Status byte read error\n");
> + goto exit_irq;
> + }
> +
> + if (status & MAX30210_PWR_RDY_MASK) {
> + dev_info(&indio_dev->dev, "power-on\n");
dev_dbg() at most.
> + st->watermark = MAX30210_WATERMARK_DEFAULT;
> + }
> +
> + if (status & MAX30210_A_FULL_MASK)
> + max30210_fifo_read(indio_dev);
> +
> + if (status & MAX30210_TEMP_HI_MASK)
This is unusual. If you have a single interrupt for both trigger
and thresholds then you can't use iio_trigger_generic_data_rdy_poll()
The main interrupt handler needs to work out what has happened then
ultimately use iio_trigger_poll_nested() to fire of the data capture.
However, there is a hardware fifo going on here. So what benefit
is the trigger providing? Triggers are optional and often not appropriate
when there are hardware fifos present because it is not useful to use
them to capture data from other devices etc. So just drop the trigger
here and have this as the main irq handler.
> + iio_push_event(indio_dev,
> + IIO_UNMOD_EVENT_CODE(IIO_TEMP, 0,
> + IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_RISING),
> + iio_get_time_ns(indio_dev));
> +
> + if (status & MAX30210_TEMP_LO_MASK)
> + iio_push_event(indio_dev,
> + IIO_UNMOD_EVENT_CODE(IIO_TEMP, 0,
> + IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_FALLING),
> + iio_get_time_ns(indio_dev));
> +
> +exit_irq:
Whilst this is not buggy the advice (see cleanup.h comments) is never
combine gotos and guard() / __free() in one function. There are some evil corner
case and GCC at least doesn't catch them all. Various ways to refactor
the code to avoid the mix.
> + iio_trigger_notify_done(indio_dev->trig);
> + return IRQ_HANDLED;
> +}
> +
> +static int max30210_validate_trigger(struct iio_dev *indio_dev,
> + struct iio_trigger *trig)
> +{
> + struct max30210_state *st = iio_priv(indio_dev);
> +
> + if (st->trig != trig)
> + return -EINVAL;
Can you use iio_validate_own_trigger()?
They should both have the same parent.
> +
> + return 0;
> +static int max30210_write_event(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + enum iio_event_info info, int val, int val2)
> +{
> + struct max30210_state *st = iio_priv(indio_dev);
> +
> + if (info == IIO_EV_INFO_VALUE) {
Can flip the logic to reduce indent.
if (info != IIO_EV_INFO_VALUE)
return -EINVAL;
switch()
> + switch (dir) {
> + case IIO_EV_DIR_RISING:
> + switch (type) {
> + case IIO_EV_TYPE_THRESH:
> + return max30210_write_temp(st->regmap,
> + MAX30210_TEMP_ALM_HI_REG, val);
> + default:
> + return -EINVAL;
> + }
> + break;
As below (check for other instances of this)
> + case IIO_EV_DIR_FALLING:
> + switch (type) {
> + case IIO_EV_TYPE_THRESH:
> + return max30210_write_temp(st->regmap,
> + MAX30210_TEMP_ALM_LO_REG, val);
> + default:
> + return -EINVAL;
> + }
> + break;
Can't get here so drop the break.
> + default:
> + return -EINVAL;
> + }
> + }
> +
> + return -EINVAL;
> +}
> +
> +static int max30210_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int *val,
> + int *val2, long mask)
> +{
> + struct max30210_state *st = iio_priv(indio_dev);
> + unsigned int uval;
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SCALE:
> + *val = 5;
> +
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + ret = regmap_read(st->regmap, MAX30210_TEMP_CONF_2_REG, &uval);
> + if (ret)
> + return ret;
> +
> + uval = FIELD_GET(MAX30210_TEMP_PERIOD_MASK, uval);
> +
> + *val = 8;
> +
> + /**
/*
See below and run a W=1 build which will probably complain about this.
> + * register values 0x9 or above have the same sample
> + * rate of 8Hz
> + */
> + *val2 = uval >= 0x9 ? 1 : BIT(0x9 - uval);
> +
> + return IIO_VAL_FRACTIONAL;
> + case IIO_CHAN_INFO_RAW:
> + ret = iio_device_claim_direct_mode(indio_dev);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(st->regmap, MAX30210_TEMP_CONV_REG,
> + MAX30210_CONV_T_MASK);
> + if (ret)
> + goto release_dmode;
> +
> + fsleep(8000);
Add a spec reference for this.
> +
> + ret = max30210_read_temp(st->regmap, MAX30210_TEMP_DATA_REG, val);
> +
> +release_dmode:
Labels for gotos inside a switch are not a good thing to do for readability.
We have the new ACQUIRE() stuff that David mentioned, but if that isn't appropriate
I'd suggest factoring out some of the code here so you can avoid the goto.
> + iio_device_release_direct_mode(indio_dev);
> + return ret;
> + default:
> + return -EINVAL;
> + }
> +}
> +}
> +
> +static int max30210_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int val,
> + int val2, long mask)
> +{
> + struct max30210_state *st = iio_priv(indio_dev);
> + u64 data;
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + ret = iio_device_claim_direct_mode(indio_dev);
Others pointed out this needs to be based on upstream where that
particular function has gone away.
> + if (ret)
> + return ret;
> +
> + /**
Not kernel-doc style so /*
> + * micro_value = val * 1000000 + val2
> + * reg_value = ((micro_value * 64) / 1000000) - 1
Except that's not the reg_val, because you then call fls() on it.
> + */
> + data = (val * MICRO + val2) << 6;
> + do_div(data, MICRO);
> +
> + data = fls_long(data - 1);
This is unusual enough I'd add a comment on the maths.
> + data = FIELD_PREP(MAX30210_TEMP_PERIOD_MASK, data);
Use a different variable of the appropriate type to store the result.
That way no need for a cast below.
> +
> + ret = regmap_update_bits(st->regmap, MAX30210_TEMP_CONF_2_REG,
> + MAX30210_TEMP_PERIOD_MASK,
> + (unsigned int)data);
> +
> + iio_device_release_direct_mode(indio_dev);
> + return ret;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static const struct iio_dev_attr *max30210_fifo_attributes[] = {
> + &iio_dev_attr_hwfifo_watermark_min,
> + &iio_dev_attr_hwfifo_watermark_max,
> + &iio_dev_attr_hwfifo_watermark,
> + NULL,
No comma for a null terminator. Doesn't add anything useful and
makes it easier to put things after this (which is obviously a bug).
> +};
> +
> +static int max30210_buffer_preenable(struct iio_dev *indio_dev)
> +{
> + struct max30210_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + ret = regmap_update_bits(st->regmap, MAX30210_INT_EN_REG,
> + MAX30210_A_FULL_MASK, MAX30210_A_FULL_MASK);
regmap_set_bits() just avoids repeating the mask.
> + if (ret)
> + return ret;
> +
> + ret = regmap_update_bits(st->regmap, MAX30210_FIFO_CONF_2_REG,
> + MAX30210_FLUSH_FIFO_MASK,
> + MAX30210_FLUSH_FIFO_MASK);
set bits is fine here. I assume it auto-clears?
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(st->regmap, MAX30210_TEMP_CONV_REG,
> + MAX30210_AUTO_MASK | MAX30210_CONV_T_MASK);
> + if (ret)
> + return ret;
> +
> + return 0;
return regmap_write()
> +}
> +
> +static int max30210_buffer_postdisable(struct iio_dev *indio_dev)
> +{
> + struct max30210_state *st = iio_priv(indio_dev);
> + int ret;
> +
> + ret = regmap_update_bits(st->regmap, MAX30210_INT_EN_REG,
> + MAX30210_A_FULL_MASK, 0x0);
Tiny bit simpler as
ret = regmap_clear_bits(st->regmap, MAX30210_INT_EN_REG,
MAX30210_A_FULL_MASK);
> + if (ret)
> + return ret;
> +
> + ret = regmap_update_bits(st->regmap, MAX30210_FIFO_CONF_2_REG,
> + MAX30210_FLUSH_FIFO_MASK,
> + MAX30210_FLUSH_FIFO_MASK);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(st->regmap, MAX30210_TEMP_CONV_REG, 0x0);
return regmap_write();
I'm a bit surprised by the ordering in here (I haven't looked at the datasheet).
Mostly we'd expect the tear down of settings to be the reverse of setup. Here
that probably means that final write belongs before the flushing of the fifo.
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +static const struct iio_buffer_setup_ops max30210_buffer_ops = {
> + .preenable = max30210_buffer_preenable,
> + .postdisable = max30210_buffer_postdisable,
> +};
> +
> +static const struct iio_info max30210_info = {
> + .read_raw = max30210_read_raw,
> + .read_avail = max30210_read_avail,
> + .write_raw = max30210_write_raw,
> + .write_raw_get_fmt = max30210_write_raw_get_fmt,
> + .hwfifo_set_watermark = max30210_set_watermark,
> + .debugfs_reg_access = &max30210_reg_access,
Why & for some function pointers and not others?
> + .validate_trigger = &max30210_validate_trigger,
> + .read_event_value = max30210_read_event,
> + .write_event_value = max30210_write_event,
> + .write_event_config = max30210_write_event_config,
> + .read_event_config = max30210_read_event_config,
> +};
> +
> +static const struct iio_chan_spec max30210_channels = {
> + .type = IIO_TEMP,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE) |
> + BIT(IIO_CHAN_INFO_SAMP_FREQ),
> + .info_mask_separate_available = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> + .output = 0,
0 is a natural default for a boolean type thing like .output, so
no need to specify it. Let the C spec guarantees around zeroing all
fields deal with it for you.
> + .scan_index = 0,
> + .event_spec = max30210_events,
> + .num_event_specs = ARRAY_SIZE(max30210_events),
> + .scan_type = {
> + .sign = 's',
> + .realbits = 16,
> + .storagebits = 32,
> + .shift = 8,
> + .endianness = IIO_BE,
> + },
> +};
> +static int max30210_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct iio_dev *indio_dev;
> + struct max30210_state *st;
> + int ret;
> +
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_BYTE_DATA))
> + return -EOPNOTSUPP;
> +
> + indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + st = iio_priv(indio_dev);
> +
> + mutex_init(&st->lock);
ret = devm_mutex_init(&st->lock);
if (ret)
return ret;
Look at what that does. We don't care a lot about lock lifetime debugging
but given it's easy to do, why not enable it for the driver.
> +
> + ret = devm_regulator_get_enable(dev, "vdd");
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "Failed to enable vdd regulator.\n");
> +
> + st->regmap = devm_regmap_init_i2c(client, &max30210_regmap);
> + if (IS_ERR(st->regmap))
> + return dev_err_probe(dev, PTR_ERR(st->regmap),
> + "Failed to allocate regmap.\n");
> +
> + ret = max30210_setup(st, dev);
> + if (ret)
> + return ret;
> +
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->channels = &max30210_channels;
> + indio_dev->num_channels = 1;
> + indio_dev->name = "max30210";
> + indio_dev->info = &max30210_info;
> +
> + ret = devm_iio_triggered_buffer_setup_ext(dev, indio_dev, NULL,
> + max30210_trigger_handler,
> + IIO_BUFFER_DIRECTION_IN,
> + &max30210_buffer_ops,
> + max30210_fifo_attributes);
> + if (ret < 0)
> + return ret;
For consistency, if (ret) should be fine here. I don't think any
IIO core calls return positive integers.
> +
> + if (client->irq) {
> + st->trig = devm_iio_trigger_alloc(dev, "%s-dev%d",
> + indio_dev->name,
> + iio_device_id(indio_dev));
> + if (!st->trig)
> + return -ENOMEM;
> +
> + st->trig->ops = &max30210_trigger_ops;
> + iio_trigger_set_drvdata(st->trig, indio_dev);
> + ret = devm_iio_trigger_register(dev, st->trig);
> + if (ret)
> + return ret;
> +
> + indio_dev->trig = st->trig;
> + ret = devm_request_threaded_irq(dev, client->irq,
> + iio_trigger_generic_data_rdy_poll,
> + NULL, IRQF_TRIGGER_FALLING,
So we have an unfortunate history of drivers that specify the interrupt
polarity that we can't fix, but for new drivers, that is a job for firmware
not the driver (because there may be an inverter in the path or similar).
Hence most likely flags here should be IRQF_NO_THREAD.
There is no thread, so devm_request_irq() is enough.
However, note that iio_trigger_generic_data_ready_poll() is ultimately kicking
of activity on an interrupt chip (software one) that is buried in the
IIO core. That can't be done correctly from a thread. Hence the IRQF_NO_THREAD.
> + indio_dev->name, st->trig);
> + if (ret)
> + return ret;
> + }
> +
> + ret = devm_iio_device_register(dev, indio_dev);
> + if (ret)
> + return ret;
> +
> + return 0;
return devm_iio_device_register();
> +}
> +
> +static const struct i2c_device_id max30210_id[] = {
> + { "max30210", 0 },
No point in the 0, so just
{ "max30210" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, max30210_id);
^ permalink raw reply [flat|nested] 11+ messages in thread