Message-ID: <20251206205407.6ba41d76@jic23-huawei>
Date: Sat, 6 Dec 2025 20:54:07 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Petre Rodan <petre.rodan@...dimension.ro>
Cc: David Lechner <dlechner@...libre.com>, Nuno Sá
 <nuno.sa@...log.com>, Andy Shevchenko <andy@...nel.org>, Rob Herring
 <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 2/2] iio: pressure: add Honeywell ABP2 driver

On Thu, 04 Dec 2025 17:12:56 +0200
Petre Rodan <petre.rodan@...dimension.ro> wrote:

> Adds driver for digital Honeywell ABP2 series of board mount
> pressure and temperature sensors.
> 
> This driver covers 113 different pressure ranges and units on
> both i2c and SPI buses.
> 
> The communication protocol involves sending two simple commands
> to the sensor and there is no register access or a memory map.
> For this reason the regmap API was not used.
> 
> The i2c address is hardcoded and depends on the part number.
> 
> Optional end of conversion interrupt control is present on the
> i2c variants of the chips.
> The EOC can also be defined for the SPI variants if a non-ABP2
> but compatible chip is to be driven.
> 
> Tested on two sensors (ABP2MRRT001PDSA3 and ABP2DANT001BA2A3).
> 
> Datasheet: https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/basic-abp2-series/documents/sps-siot-abp2-series-datasheet-32350268-en.pdf
> Signed-off-by: Petre Rodan <petre.rodan@...dimension.ro>

Hi Petre

Nice clean driver.

A few things inline.  I managed to delete my first review so the comments are a terse
memory of what I had in that!  End of review sessions and dinner time for me
so sorry for the brevity!

Jonathan

> diff --git a/drivers/iio/pressure/abp2030pa.c b/drivers/iio/pressure/abp2030pa.c
> new file mode 100644
> index 000000000000..80623043bd55
> --- /dev/null
> +++ b/drivers/iio/pressure/abp2030pa.c
> @@ -0,0 +1,539 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Honeywell ABP2 series pressure sensor driver
> + *
> + * Copyright (c) 2025 Petre Rodan <petre.rodan@...dimension.ro>
> + *
> + * Datasheet: https://prod-edam.honeywell.com/content/dam/honeywell-edam/sps/siot/en-us/products/sensors/pressure-sensors/board-mount-pressure-sensors/basic-abp2-series/documents/sps-siot-abp2-series-datasheet-32350268-en.pdf
> + */
> +
> +#include <linux/array_size.h>
> +#include <linux/bits.h>
> +#include <linux/completion.h>
> +#include <linux/delay.h>
> +#include <linux/dev_printk.h>
> +#include <linux/device.h>
> +#include <linux/errno.h>
> +#include <linux/export.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/interrupt.h>
> +#include <linux/jiffies.h>

I'm not immediately spotting use of this but I might well be missing it!

> +#include <linux/math64.h>
> +#include <linux/module.h>
> +#include <linux/property.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/string.h>
> +#include <linux/time.h>
> +#include <linux/types.h>
> +#include <linux/unaligned.h>
> +#include <linux/units.h>
> +
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>

This is rarely used in a modern IIO driver. Why do you need it here?

> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>
>
> +
> +static irqreturn_t abp2_trigger_handler(int irq, void *private)
> +{
> +	int ret;
> +	struct iio_poll_func *pf = private;
> +	struct iio_dev *indio_dev = pf->indio_dev;
> +	struct abp2_data *data = iio_priv(indio_dev);
> +
> +	ret = abp2_get_measurement(data);
> +	if (ret < 0)
> +		goto out_notify_done;
> +
> +	data->scan.chan[0] = get_unaligned_be24(&data->rx_buf[1]);
> +	data->scan.chan[1] = get_unaligned_be24(&data->rx_buf[4]);
Turn on just the second channel and run the test in tools/iio/ for buffered
reads.

You'll see data from the first channel not the one userspace asked for.

Set available_scan_masks = { 0x3, 0 };
allows the IIO core to know this is what is happening and shuffle the
data as necessary for you.

> +
> +	iio_push_to_buffers_with_ts(indio_dev, &data->scan, sizeof(data->scan),
> +				    iio_get_time_ns(indio_dev));
> +
> +out_notify_done:
> +	iio_trigger_notify_done(indio_dev->trig);
> +
> +	return IRQ_HANDLED;
> +}

> +int abp2_common_probe(struct device *dev, const struct abp2_ops *ops, int irq)
> +{
> +	int ret;
> +	struct abp2_data *data;
> +	struct iio_dev *indio_dev;
> +	const char *triplet;
> +	s32 tmp;
> +	s64 odelta, pdelta;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	data = iio_priv(indio_dev);
> +	data->dev = dev;
> +	data->ops = ops;
> +	data->irq = irq;
> +
> +	init_completion(&data->completion);
> +
> +	indio_dev->name = "abp2030pa";
> +	indio_dev->info = &abp2_info;
> +	indio_dev->channels = abp2_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(abp2_channels);
Put back setting direct mode (see comment I made in reply to that earlier thread)



> diff --git a/drivers/iio/pressure/abp2030pa.h b/drivers/iio/pressure/abp2030pa.h
> new file mode 100644
> index 000000000000..2beb7306812d
> --- /dev/null
> +++ b/drivers/iio/pressure/abp2030pa.h

> +/**
> + * struct abp2_data
> + * @dev: current device structure
> + * @ops: pointers for bus specific read and write functions
> + * @pmin: minimal pressure in pascal
> + * @pmax: maximal pressure in pascal
> + * @outmin: minimum raw pressure in counts (based on transfer function)
> + * @outmax: maximum raw pressure in counts (based on transfer function)
> + * @function: transfer function
> + * @p_scale: pressure scale
> + * @p_scale_dec: pressure scale, decimal number
> + * @p_offset: pressure offset
> + * @irq: end of conversion - applies only to the i2c sensor
> + * @completion: handshake from irq to read
> + * @scan: channel values for buffered mode
> + * @tx_buf: transmit buffer used during the SPI communication
> + * @rx_buf: raw data provided by sensor
> + */
> +struct abp2_data {
> +	struct device *dev;
> +	const struct abp2_ops *ops;
> +	s32 pmin;
> +	s32 pmax;
> +	u32 outmin;
> +	u32 outmax;
> +	enum abp2_func_id function;
> +	int p_scale;
> +	int p_scale_dec;
> +	int p_offset;
> +	int irq;
> +	struct completion completion;
> +	struct {
> +		u32 chan[2];
> +		aligned_s64 timestamp;
> +	} scan;
> +	u8 tx_buf[ABP2_MEASUREMENT_RD_SIZE] __aligned(IIO_DMA_MINALIGN);
> +	u8 rx_buf[ABP2_MEASUREMENT_RD_SIZE] __aligned(IIO_DMA_MINALIGN);

See reply to earlier version (I've been away so lots of catching up to do!)

> +};

> diff --git a/drivers/iio/pressure/abp2030pa_spi.c b/drivers/iio/pressure/abp2030pa_spi.c
> new file mode 100644
> index 000000000000..924fcb2fb690
> --- /dev/null
> +++ b/drivers/iio/pressure/abp2030pa_spi.c
> @@ -0,0 +1,67 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Honeywell ABP2 series pressure sensor driver
> + *
> + * Copyright (c) 2025 Petre Rodan <petre.rodan@...dimension.ro>
> + */
> +
> +#include <linux/errno.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/spi/spi.h>
> +#include <linux/types.h>
> +
> +#include "abp2030pa.h"
> +
> +static int abp2_spi_xfer(struct abp2_data *data, u8 cmd, u8 nbytes)
> +{
> +	struct spi_device *spi = to_spi_device(data->dev);
> +	struct spi_transfer xfer;

struct spi_transfer xfer = { };

otherwise you are at the mercy of random data on the stack.

> +
> +	if (nbytes > ABP2_MEASUREMENT_RD_SIZE)
> +		return -EOVERFLOW;
> +
> +	data->tx_buf[0] = cmd;
> +	xfer.tx_buf = data->tx_buf;
> +	xfer.rx_buf = data->rx_buf;
> +	xfer.len = nbytes;
> +
> +	return spi_sync_transfer(spi, &xfer, 1);
> +}

Powered by blists - more mailing lists