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 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 gpio can also be defined for the SPI variants if a
non-ABP2 but compatible chip is to be driven.
Tested on two different sensors.
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@subdimension.ro>
---
MAINTAINERS | 1 +
drivers/iio/pressure/Kconfig | 24 ++
drivers/iio/pressure/Makefile | 3 +
drivers/iio/pressure/abp2030pa.c | 543 +++++++++++++++++++++++++++++++++++
drivers/iio/pressure/abp2030pa.h | 79 +++++
drivers/iio/pressure/abp2030pa_i2c.c | 93 ++++++
drivers/iio/pressure/abp2030pa_spi.c | 88 ++++++
7 files changed, 831 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 15b92300acbc..0aa9cba68137 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11409,6 +11409,7 @@ M: Petre Rodan <petre.rodan@subdimension.ro>
L: linux-iio@vger.kernel.org
S: Maintained
F: Documentation/devicetree/bindings/iio/pressure/honeywell,abp2030pa.yaml
+F: drivers/iio/pressure/abp2030pa*
HONEYWELL HSC030PA PRESSURE SENSOR SERIES IIO DRIVER
M: Petre Rodan <petre.rodan@subdimension.ro>
diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig
index 2fe9dc90cceb..0c2be090e27d 100644
--- a/drivers/iio/pressure/Kconfig
+++ b/drivers/iio/pressure/Kconfig
@@ -16,6 +16,30 @@ config ABP060MG
To compile this driver as a module, choose M here: the module
will be called abp060mg.
+config ABP2030PA
+ tristate "Honeywell ABP2 pressure sensor series"
+ depends on (I2C || SPI_MASTER)
+ select ABP2030PA_I2C if I2C
+ select ABP2030PA_SPI if SPI_MASTER
+ select IIO_BUFFER
+ select IIO_TRIGGERED_BUFFER
+ help
+ Say Y here to build support for the Honeywell ABP2 board mount
+ pressure and temperature sensor series.
+
+ To compile this driver as a module, choose M here: the module
+ will be called abp2030pa.
+
+config ABP2030PA_I2C
+ tristate
+ depends on ABP2030PA
+ depends on I2C
+
+config ABP2030PA_SPI
+ tristate
+ depends on ABP2030PA
+ depends on SPI_MASTER
+
config ROHM_BM1390
tristate "ROHM BM1390GLV-Z pressure sensor driver"
depends on I2C
diff --git a/drivers/iio/pressure/Makefile b/drivers/iio/pressure/Makefile
index a21443e992b9..bc0d11a20acc 100644
--- a/drivers/iio/pressure/Makefile
+++ b/drivers/iio/pressure/Makefile
@@ -5,6 +5,9 @@
# When adding new entries keep the list in alphabetical order
obj-$(CONFIG_ABP060MG) += abp060mg.o
+obj-$(CONFIG_ABP2030PA) += abp2030pa.o
+obj-$(CONFIG_ABP2030PA_I2C) += abp2030pa_i2c.o
+obj-$(CONFIG_ABP2030PA_SPI) += abp2030pa_spi.o
obj-$(CONFIG_ADP810) += adp810.o
obj-$(CONFIG_ROHM_BM1390) += rohm-bm1390.o
obj-$(CONFIG_BMP280) += bmp280.o
diff --git a/drivers/iio/pressure/abp2030pa.c b/drivers/iio/pressure/abp2030pa.c
new file mode 100644
index 000000000000..32aa11e4cefd
--- /dev/null
+++ b/drivers/iio/pressure/abp2030pa.c
@@ -0,0 +1,543 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Honeywell ABP2 series pressure sensor driver
+ *
+ * Copyright (c) 2025 Petre Rodan <petre.rodan@subdimension.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/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/math64.h>
+#include <linux/module.h>
+#include <linux/property.h>
+#include <linux/regulator/consumer.h>
+#include <linux/string.h>
+#include <linux/types.h>
+#include <linux/units.h>
+
+#include <linux/unaligned.h>
+
+#include <linux/iio/buffer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+
+#include "abp2030pa.h"
+
+/* status byte flags */
+#define ABP2_ST_POWER BIT(6) /* 1 if device is powered */
+#define ABP2_ST_BUSY BIT(5) /* 1 if device is busy */
+
+struct abp2_func_spec {
+ u32 output_min;
+ u32 output_max;
+};
+
+/*
+ * transfer function A: 10% to 90% of 2^24
+ */
+static const struct abp2_func_spec abp2_func_spec[] = {
+ [ABP2_FUNCTION_A] = { .output_min = 1677722, .output_max = 15099494 },
+};
+
+enum abp2_variants {
+ ABP2001BA = 0x0, ABP21_6BA = 0x1, ABP22_5BA = 0x2, ABP2004BA = 0x3,
+ ABP2006BA = 0x4, ABP2008BA = 0x5, ABP2010BA = 0x6, ABP2012BA = 0x7,
+ ABP2001BD = 0x8, ABP21_6BD = 0x9, ABP22_5BD = 0xa, ABP2004BD = 0xb,
+ ABP2001BG = 0xc, ABP21_6BG = 0xd, ABP22_5BG = 0xe, ABP2004BG = 0xf,
+ ABP2006BG = 0x10, ABP2008BG = 0x11, ABP2010BG = 0x12, ABP2012BG = 0x13,
+ ABP2001GG = 0x14, ABP21_2GG = 0x15, ABP2100KA = 0x16, ABP2160KA = 0x17,
+ ABP2250KA = 0x18, ABP2001KD = 0x19, ABP21_6KD = 0x1a, ABP22_5KD = 0x1b,
+ ABP2004KD = 0x1c, ABP2006KD = 0x1d, ABP2010KD = 0x1e, ABP2016KD = 0x1f,
+ ABP2025KD = 0x20, ABP2040KD = 0x21, ABP2060KD = 0x22, ABP2100KD = 0x23,
+ ABP2160KD = 0x24, ABP2250KD = 0x25, ABP2400KD = 0x26, ABP2001KG = 0x27,
+ ABP21_6KG = 0x28, ABP22_5KG = 0x29, ABP2004KG = 0x2a, ABP2006KG = 0x2b,
+ ABP2010KG = 0x2c, ABP2016KG = 0x2d, ABP2025KG = 0x2e, ABP2040KG = 0x2f,
+ ABP2060KG = 0x30, ABP2100KG = 0x31, ABP2160KG = 0x32, ABP2250KG = 0x33,
+ ABP2400KG = 0x34, ABP2600KG = 0x35, ABP2800KG = 0x36, ABP2250LD = 0x37,
+ ABP2600LD = 0x38, ABP2600LG = 0x39, ABP22_5MD = 0x3a, ABP2006MD = 0x3b,
+ ABP2010MD = 0x3c, ABP2016MD = 0x3d, ABP2025MD = 0x3e, ABP2040MD = 0x3f,
+ ABP2060MD = 0x40, ABP2100MD = 0x41, ABP2160MD = 0x42, ABP2250MD = 0x43,
+ ABP2400MD = 0x44, ABP2600MD = 0x45, ABP2006MG = 0x46, ABP2010MG = 0x47,
+ ABP2016MG = 0x48, ABP2025MG = 0x49, ABP2040MG = 0x4a, ABP2060MG = 0x4b,
+ ABP2100MG = 0x4c, ABP2160MG = 0x4d, ABP2250MG = 0x4e, ABP2400MG = 0x4f,
+ ABP2600MG = 0x50, ABP2001ND = 0x51, ABP2002ND = 0x52, ABP2004ND = 0x53,
+ ABP2005ND = 0x54, ABP2010ND = 0x55, ABP2020ND = 0x56, ABP2030ND = 0x57,
+ ABP2002NG = 0x58, ABP2004NG = 0x59, ABP2005NG = 0x5a, ABP2010NG = 0x5b,
+ ABP2020NG = 0x5c, ABP2030NG = 0x5d, ABP2015PA = 0x5e, ABP2030PA = 0x5f,
+ ABP2060PA = 0x60, ABP2100PA = 0x61, ABP2150PA = 0x62, ABP2175PA = 0x63,
+ ABP2001PD = 0x64, ABP2005PD = 0x65, ABP2015PD = 0x66, ABP2030PD = 0x67,
+ ABP2060PD = 0x68, ABP2001PG = 0x69, ABP2005PG = 0x6a, ABP2015PG = 0x6b,
+ ABP2030PG = 0x6c, ABP2060PG = 0x6d, ABP2100PG = 0x6e, ABP2150PG = 0x6f,
+ ABP2175PG = 0x70, ABP2_VARIANTS_MAX
+};
+
+static const char * const abp2_triplet_variants[ABP2_VARIANTS_MAX] = {
+ [ABP2001BA] = "001BA", [ABP21_6BA] = "1.6BA", [ABP22_5BA] = "2.5BA",
+ [ABP2004BA] = "004BA", [ABP2006BA] = "006BA", [ABP2008BA] = "008BA",
+ [ABP2010BA] = "010BA", [ABP2012BA] = "012BA", [ABP2001BD] = "001BD",
+ [ABP21_6BD] = "1.6BD", [ABP22_5BD] = "2.5BD", [ABP2004BD] = "004BD",
+ [ABP2001BG] = "001BG", [ABP21_6BG] = "1.6BG", [ABP22_5BG] = "2.5BG",
+ [ABP2004BG] = "004BG", [ABP2006BG] = "006BG", [ABP2008BG] = "008BG",
+ [ABP2010BG] = "010BG", [ABP2012BG] = "012BG", [ABP2001GG] = "001GG",
+ [ABP21_2GG] = "1.2GG", [ABP2100KA] = "100KA", [ABP2160KA] = "160KA",
+ [ABP2250KA] = "250KA", [ABP2001KD] = "001KD", [ABP21_6KD] = "1.6KD",
+ [ABP22_5KD] = "2.5KD", [ABP2004KD] = "004KD", [ABP2006KD] = "006KD",
+ [ABP2010KD] = "010KD", [ABP2016KD] = "016KD", [ABP2025KD] = "025KD",
+ [ABP2040KD] = "040KD", [ABP2060KD] = "060KD", [ABP2100KD] = "100KD",
+ [ABP2160KD] = "160KD", [ABP2250KD] = "250KD", [ABP2400KD] = "400KD",
+ [ABP2001KG] = "001KG", [ABP21_6KG] = "1.6KG", [ABP22_5KG] = "2.5KG",
+ [ABP2004KG] = "004KG", [ABP2006KG] = "006KG", [ABP2010KG] = "010KG",
+ [ABP2016KG] = "016KG", [ABP2025KG] = "025KG", [ABP2040KG] = "040KG",
+ [ABP2060KG] = "060KG", [ABP2100KG] = "100KG", [ABP2160KG] = "160KG",
+ [ABP2250KG] = "250KG", [ABP2400KG] = "400KG", [ABP2600KG] = "600KG",
+ [ABP2800KG] = "800KG", [ABP2250LD] = "250LD", [ABP2600LD] = "600LD",
+ [ABP2600LG] = "600LG", [ABP22_5MD] = "2.5MD", [ABP2006MD] = "006MD",
+ [ABP2010MD] = "010MD", [ABP2016MD] = "016MD", [ABP2025MD] = "025MD",
+ [ABP2040MD] = "040MD", [ABP2060MD] = "060MD", [ABP2100MD] = "100MD",
+ [ABP2160MD] = "160MD", [ABP2250MD] = "250MD", [ABP2400MD] = "400MD",
+ [ABP2600MD] = "600MD", [ABP2006MG] = "006MG", [ABP2010MG] = "010MG",
+ [ABP2016MG] = "016MG", [ABP2025MG] = "025MG", [ABP2040MG] = "040MG",
+ [ABP2060MG] = "060MG", [ABP2100MG] = "100MG", [ABP2160MG] = "160MG",
+ [ABP2250MG] = "250MG", [ABP2400MG] = "400MG", [ABP2600MG] = "600MG",
+ [ABP2001ND] = "001ND", [ABP2002ND] = "002ND", [ABP2004ND] = "004ND",
+ [ABP2005ND] = "005ND", [ABP2010ND] = "010ND", [ABP2020ND] = "020ND",
+ [ABP2030ND] = "030ND", [ABP2002NG] = "002NG", [ABP2004NG] = "004NG",
+ [ABP2005NG] = "005NG", [ABP2010NG] = "010NG", [ABP2020NG] = "020NG",
+ [ABP2030NG] = "030NG", [ABP2015PA] = "015PA", [ABP2030PA] = "030PA",
+ [ABP2060PA] = "060PA", [ABP2100PA] = "100PA", [ABP2150PA] = "150PA",
+ [ABP2175PA] = "175PA", [ABP2001PD] = "001PD", [ABP2005PD] = "005PD",
+ [ABP2015PD] = "015PD", [ABP2030PD] = "030PD", [ABP2060PD] = "060PD",
+ [ABP2001PG] = "001PG", [ABP2005PG] = "005PG", [ABP2015PG] = "015PG",
+ [ABP2030PG] = "030PG", [ABP2060PG] = "060PG", [ABP2100PG] = "100PG",
+ [ABP2150PG] = "150PG", [ABP2175PG] = "175PG"
+};
+
+/**
+ * struct abp2_range_config - list of pressure ranges based on nomenclature
+ * @pmin: lowest pressure that can be measured
+ * @pmax: highest pressure that can be measured
+ */
+struct abp2_range_config {
+ const s32 pmin;
+ const s32 pmax;
+};
+
+/* All min max limits have been converted to pascals */
+static const struct abp2_range_config abp2_range_config[ABP2_VARIANTS_MAX] = {
+ [ABP2001BA] = { .pmin = 0, .pmax = 100000 },
+ [ABP21_6BA] = { .pmin = 0, .pmax = 160000 },
+ [ABP22_5BA] = { .pmin = 0, .pmax = 250000 },
+ [ABP2004BA] = { .pmin = 0, .pmax = 400000 },
+ [ABP2006BA] = { .pmin = 0, .pmax = 600000 },
+ [ABP2008BA] = { .pmin = 0, .pmax = 800000 },
+ [ABP2010BA] = { .pmin = 0, .pmax = 1000000 },
+ [ABP2012BA] = { .pmin = 0, .pmax = 1200000 },
+ [ABP2001BD] = { .pmin = -100000, .pmax = 100000 },
+ [ABP21_6BD] = { .pmin = -160000, .pmax = 160000 },
+ [ABP22_5BD] = { .pmin = -250000, .pmax = 250000 },
+ [ABP2004BD] = { .pmin = -400000, .pmax = 400000 },
+ [ABP2001BG] = { .pmin = 0, .pmax = 100000 },
+ [ABP21_6BG] = { .pmin = 0, .pmax = 160000 },
+ [ABP22_5BG] = { .pmin = 0, .pmax = 250000 },
+ [ABP2004BG] = { .pmin = 0, .pmax = 400000 },
+ [ABP2006BG] = { .pmin = 0, .pmax = 600000 },
+ [ABP2008BG] = { .pmin = 0, .pmax = 800000 },
+ [ABP2010BG] = { .pmin = 0, .pmax = 1000000 },
+ [ABP2012BG] = { .pmin = 0, .pmax = 1200000 },
+ [ABP2001GG] = { .pmin = 0, .pmax = 1000000 },
+ [ABP21_2GG] = { .pmin = 0, .pmax = 1200000 },
+ [ABP2100KA] = { .pmin = 0, .pmax = 100000 },
+ [ABP2160KA] = { .pmin = 0, .pmax = 160000 },
+ [ABP2250KA] = { .pmin = 0, .pmax = 250000 },
+ [ABP2001KD] = { .pmin = -1000, .pmax = 1000 },
+ [ABP21_6KD] = { .pmin = -1600, .pmax = 1600 },
+ [ABP22_5KD] = { .pmin = -2500, .pmax = 2500 },
+ [ABP2004KD] = { .pmin = -4000, .pmax = 4000 },
+ [ABP2006KD] = { .pmin = -6000, .pmax = 6000 },
+ [ABP2010KD] = { .pmin = -10000, .pmax = 10000 },
+ [ABP2016KD] = { .pmin = -16000, .pmax = 16000 },
+ [ABP2025KD] = { .pmin = -25000, .pmax = 25000 },
+ [ABP2040KD] = { .pmin = -40000, .pmax = 40000 },
+ [ABP2060KD] = { .pmin = -60000, .pmax = 60000 },
+ [ABP2100KD] = { .pmin = -100000, .pmax = 100000 },
+ [ABP2160KD] = { .pmin = -160000, .pmax = 160000 },
+ [ABP2250KD] = { .pmin = -250000, .pmax = 250000 },
+ [ABP2400KD] = { .pmin = -400000, .pmax = 400000 },
+ [ABP2001KG] = { .pmin = 0, .pmax = 1000 },
+ [ABP21_6KG] = { .pmin = 0, .pmax = 1600 },
+ [ABP22_5KG] = { .pmin = 0, .pmax = 2500 },
+ [ABP2004KG] = { .pmin = 0, .pmax = 4000 },
+ [ABP2006KG] = { .pmin = 0, .pmax = 6000 },
+ [ABP2010KG] = { .pmin = 0, .pmax = 10000 },
+ [ABP2016KG] = { .pmin = 0, .pmax = 16000 },
+ [ABP2025KG] = { .pmin = 0, .pmax = 25000 },
+ [ABP2040KG] = { .pmin = 0, .pmax = 40000 },
+ [ABP2060KG] = { .pmin = 0, .pmax = 60000 },
+ [ABP2100KG] = { .pmin = 0, .pmax = 100000 },
+ [ABP2160KG] = { .pmin = 0, .pmax = 160000 },
+ [ABP2250KG] = { .pmin = 0, .pmax = 250000 },
+ [ABP2400KG] = { .pmin = 0, .pmax = 400000 },
+ [ABP2600KG] = { .pmin = 0, .pmax = 600000 },
+ [ABP2800KG] = { .pmin = 0, .pmax = 800000 },
+ [ABP2250LD] = { .pmin = -250, .pmax = 250 },
+ [ABP2600LD] = { .pmin = -600, .pmax = 600 },
+ [ABP2600LG] = { .pmin = 0, .pmax = 600 },
+ [ABP22_5MD] = { .pmin = -250, .pmax = 250 },
+ [ABP2006MD] = { .pmin = -600, .pmax = 600 },
+ [ABP2010MD] = { .pmin = -1000, .pmax = 1000 },
+ [ABP2016MD] = { .pmin = -1600, .pmax = 1600 },
+ [ABP2025MD] = { .pmin = -2500, .pmax = 2500 },
+ [ABP2040MD] = { .pmin = -4000, .pmax = 4000 },
+ [ABP2060MD] = { .pmin = -6000, .pmax = 6000 },
+ [ABP2100MD] = { .pmin = -10000, .pmax = 10000 },
+ [ABP2160MD] = { .pmin = -16000, .pmax = 16000 },
+ [ABP2250MD] = { .pmin = -25000, .pmax = 25000 },
+ [ABP2400MD] = { .pmin = -40000, .pmax = 40000 },
+ [ABP2600MD] = { .pmin = -60000, .pmax = 60000 },
+ [ABP2006MG] = { .pmin = 0, .pmax = 600 },
+ [ABP2010MG] = { .pmin = 0, .pmax = 1000 },
+ [ABP2016MG] = { .pmin = 0, .pmax = 1600 },
+ [ABP2025MG] = { .pmin = 0, .pmax = 2500 },
+ [ABP2040MG] = { .pmin = 0, .pmax = 4000 },
+ [ABP2060MG] = { .pmin = 0, .pmax = 6000 },
+ [ABP2100MG] = { .pmin = 0, .pmax = 10000 },
+ [ABP2160MG] = { .pmin = 0, .pmax = 16000 },
+ [ABP2250MG] = { .pmin = 0, .pmax = 25000 },
+ [ABP2400MG] = { .pmin = 0, .pmax = 40000 },
+ [ABP2600MG] = { .pmin = 0, .pmax = 60000 },
+ [ABP2001ND] = { .pmin = -249, .pmax = 249 },
+ [ABP2002ND] = { .pmin = -498, .pmax = 498 },
+ [ABP2004ND] = { .pmin = -996, .pmax = 996 },
+ [ABP2005ND] = { .pmin = -1245, .pmax = 1245 },
+ [ABP2010ND] = { .pmin = -2491, .pmax = 2491 },
+ [ABP2020ND] = { .pmin = -4982, .pmax = 4982 },
+ [ABP2030ND] = { .pmin = -7473, .pmax = 7473 },
+ [ABP2002NG] = { .pmin = 0, .pmax = 498 },
+ [ABP2004NG] = { .pmin = 0, .pmax = 996 },
+ [ABP2005NG] = { .pmin = 0, .pmax = 1245 },
+ [ABP2010NG] = { .pmin = 0, .pmax = 2491 },
+ [ABP2020NG] = { .pmin = 0, .pmax = 4982 },
+ [ABP2030NG] = { .pmin = 0, .pmax = 7473 },
+ [ABP2015PA] = { .pmin = 0, .pmax = 103421 },
+ [ABP2030PA] = { .pmin = 0, .pmax = 206843 },
+ [ABP2060PA] = { .pmin = 0, .pmax = 413685 },
+ [ABP2100PA] = { .pmin = 0, .pmax = 689476 },
+ [ABP2150PA] = { .pmin = 0, .pmax = 1034214 },
+ [ABP2175PA] = { .pmin = 0, .pmax = 1206583 },
+ [ABP2001PD] = { .pmin = -6895, .pmax = 6895 },
+ [ABP2005PD] = { .pmin = -34474, .pmax = 34474 },
+ [ABP2015PD] = { .pmin = -103421, .pmax = 103421 },
+ [ABP2030PD] = { .pmin = -206843, .pmax = 206843 },
+ [ABP2060PD] = { .pmin = -413685, .pmax = 413685 },
+ [ABP2001PG] = { .pmin = 0, .pmax = 6895 },
+ [ABP2005PG] = { .pmin = 0, .pmax = 34474 },
+ [ABP2015PG] = { .pmin = 0, .pmax = 103421 },
+ [ABP2030PG] = { .pmin = 0, .pmax = 206843 },
+ [ABP2060PG] = { .pmin = 0, .pmax = 413685 },
+ [ABP2100PG] = { .pmin = 0, .pmax = 689476 },
+ [ABP2150PG] = { .pmin = 0, .pmax = 1034214 },
+ [ABP2175PG] = { .pmin = 0, .pmax = 1206583 }
+};
+
+static int abp2_get_measurement(struct abp2_data *data)
+{
+ struct device *dev = data->dev;
+ int ret;
+
+ memset(data->buffer, 0, sizeof(data->buffer));
+ reinit_completion(&data->completion);
+
+ ret = data->ops->write(data, ABP2_CMD_SYNC, ABP2_PKT_SYNC_LEN);
+ if (ret < 0)
+ return ret;
+
+ if (data->irq > 0) {
+ ret = wait_for_completion_timeout(&data->completion, HZ);
+ if (!ret) {
+ dev_err(dev, "timeout waiting for EOC interrupt\n");
+ return -ETIMEDOUT;
+ }
+ } else
+ fsleep(5000);
+
+ ret = data->ops->read(data, ABP2_CMD_NOP, ABP2_PKT_NOP_LEN);
+ if (ret < 0)
+ return ret;
+
+ /*
+ * status byte flags
+ * bit7 SANITY_CHK - must always be 0
+ * bit6 ABP2_ST_POWER - 1 if device is powered
+ * bit5 ABP2_ST_BUSY - 1 if device has no new conversion ready
+ * bit4 SANITY_CHK - must always be 0
+ * bit3 SANITY_CHK - must always be 0
+ * bit2 MEMORY_ERR - 1 if integrity test has failed
+ * bit1 SANITY_CHK - must always be 0
+ * bit0 MATH_ERR - 1 during internal math saturation err
+ */
+
+ if (data->buffer[0] == (ABP2_ST_POWER | ABP2_ST_BUSY))
+ return -ETIMEDOUT;
+
+ if (data->buffer[0] != ABP2_ST_POWER) {
+ dev_err(data->dev,
+ "unexpected status byte 0x%02x\n", data->buffer[0]);
+ return -ETIMEDOUT;
+ }
+
+ return 0;
+}
+
+static irqreturn_t abp2_eoc_handler(int irq, void *private)
+{
+ struct abp2_data *data = private;
+
+ complete(&data->completion);
+
+ return IRQ_HANDLED;
+}
+
+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 err;
+
+ data->scan.chan[0] = get_unaligned_be24(&data->buffer[1]);
+ data->scan.chan[1] = get_unaligned_be24(&data->buffer[4]);
+
+ iio_push_to_buffers_with_ts(indio_dev, &data->scan, sizeof(data->scan),
+ iio_get_time_ns(indio_dev));
+
+err:
+ iio_trigger_notify_done(indio_dev->trig);
+
+ return IRQ_HANDLED;
+}
+
+/*
+ * IIO ABI expects
+ * value = (conv + offset) * scale
+ *
+ * datasheet provides the following formula for determining the temperature
+ * temp[C] = conv * a + b
+ * where a = 200/16777215; b = -50
+ *
+ * temp[C] = (conv + (b/a)) * a * (1000)
+ * =>
+ * scale = a * 1000 = .0000119209296 * 1000 = .01192092966562
+ * offset = b/a = -50 * 16777215 / 200 = -4194303.75
+ *
+ * based on the datasheet
+ * pressure = (conv - Omin) * Q + Pmin =
+ * ((conv - Omin) + Pmin/Q) * Q
+ * =>
+ * scale = Q = (Pmax - Pmin) / (Omax - Omin)
+ * offset = Pmin/Q - Omin = Pmin * (Omax - Omin) / (Pmax - Pmin) - Omin
+ */
+static int abp2_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *channel, int *val,
+ int *val2, long mask)
+{
+ struct abp2_data *data = iio_priv(indio_dev);
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ ret = abp2_get_measurement(data);
+ if (ret < 0)
+ return ret;
+
+ switch (channel->type) {
+ case IIO_PRESSURE:
+ *val = get_unaligned_be24(&data->buffer[1]);
+ return IIO_VAL_INT;
+ case IIO_TEMP:
+ *val = get_unaligned_be24(&data->buffer[4]);
+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
+ }
+ return IIO_VAL_INT;
+
+ case IIO_CHAN_INFO_SCALE:
+ switch (channel->type) {
+ case IIO_TEMP:
+ *val = 0;
+ *val2 = 11920929;
+ return IIO_VAL_INT_PLUS_NANO;
+ case IIO_PRESSURE:
+ *val = data->p_scale;
+ *val2 = data->p_scale_dec;
+ return IIO_VAL_INT_PLUS_NANO;
+ default:
+ return -EINVAL;
+ }
+
+ case IIO_CHAN_INFO_OFFSET:
+ switch (channel->type) {
+ case IIO_TEMP:
+ *val = -4194304;
+ return IIO_VAL_INT;
+ case IIO_PRESSURE:
+ *val = data->p_offset;
+ return IIO_VAL_INT;
+ default:
+ return -EINVAL;
+ }
+
+ default:
+ return -EINVAL;
+ }
+}
+
+static const struct iio_info abp2_info = {
+ .read_raw = &abp2_read_raw,
+};
+
+static const struct iio_chan_spec abp2_channels[] = {
+ {
+ .type = IIO_PRESSURE,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+ BIT(IIO_CHAN_INFO_SCALE) |
+ BIT(IIO_CHAN_INFO_OFFSET),
+ .scan_index = 0,
+ .scan_type = {
+ .sign = 'u',
+ .realbits = 24,
+ .storagebits = 32,
+ .endianness = IIO_CPU,
+ },
+ },
+ {
+ .type = IIO_TEMP,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+ BIT(IIO_CHAN_INFO_SCALE) |
+ BIT(IIO_CHAN_INFO_OFFSET),
+ .scan_index = 1,
+ .scan_type = {
+ .sign = 'u',
+ .realbits = 24,
+ .storagebits = 32,
+ .endianness = IIO_CPU,
+ },
+ },
+ IIO_CHAN_SOFT_TIMESTAMP(2),
+};
+
+
+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;
+ s64 tmp;
+
+ 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);
+ indio_dev->modes = INDIO_DIRECT_MODE;
+
+ ret = devm_regulator_get_enable(dev, "vdd");
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "can't get and enable vdd supply\n");
+
+ ret = data->ops->init(data->dev);
+ if (ret)
+ return ret;
+
+ ret = device_property_read_string(dev, "honeywell,pressure-triplet",
+ &triplet);
+ if (ret) {
+ ret = device_property_read_u32(dev, "honeywell,pmin-pascal",
+ &data->pmin);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "honeywell,pmin-pascal could not be read\n");
+
+ ret = device_property_read_u32(dev, "honeywell,pmax-pascal",
+ &data->pmax);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "honeywell,pmax-pascal could not be read\n");
+ } else {
+ ret = device_property_match_property_string(dev,
+ "honeywell,pressure-triplet",
+ abp2_triplet_variants,
+ ABP2_VARIANTS_MAX);
+ if (ret < 0)
+ return dev_err_probe(dev, -EINVAL,
+ "honeywell,pressure-triplet is invalid\n");
+
+ data->pmin = abp2_range_config[ret].pmin;
+ data->pmax = abp2_range_config[ret].pmax;
+ }
+
+ if (data->pmin >= data->pmax)
+ return dev_err_probe(dev, -EINVAL,
+ "pressure limits are invalid\n");
+
+ data->outmin = abp2_func_spec[data->function].output_min;
+ data->outmax = abp2_func_spec[data->function].output_max;
+
+ tmp = div_s64(((s64)(data->pmax - data->pmin)) * NANO,
+ data->outmax - data->outmin);
+ data->p_scale = div_s64_rem(tmp, NANO, &data->p_scale_dec);
+
+ tmp = div_s64((s64)data->pmin * (s64)(data->outmax - data->outmin),
+ (s64)(data->pmax - data->pmin)) - (s64)(data->outmin);
+ data->p_offset = tmp;
+
+ if (data->irq > 0) {
+ ret = devm_request_irq(dev, data->irq, abp2_eoc_handler,
+ IRQF_TRIGGER_RISING,
+ dev_name(dev),
+ data);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "request irq %d failed\n", data->irq);
+ }
+
+ ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
+ abp2_trigger_handler, NULL);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "iio triggered buffer setup failed\n");
+
+ ret = devm_iio_device_register(dev, indio_dev);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "unable to register iio device\n");
+
+ return 0;
+}
+EXPORT_SYMBOL_NS(abp2_common_probe, "IIO_HONEYWELL_ABP2030PA");
+
+MODULE_AUTHOR("Petre Rodan <petre.rodan@subdimension.ro>");
+MODULE_DESCRIPTION("Honeywell ABP2 pressure sensor core driver");
+MODULE_LICENSE("GPL");
diff --git a/drivers/iio/pressure/abp2030pa.h b/drivers/iio/pressure/abp2030pa.h
new file mode 100644
index 000000000000..14f55e9b3869
--- /dev/null
+++ b/drivers/iio/pressure/abp2030pa.h
@@ -0,0 +1,79 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Honeywell ABP2 series pressure sensor driver
+ *
+ * Copyright (c) 2025 Petre Rodan <petre.rodan@subdimension.ro>
+ */
+
+#ifndef _ABP2030PA_H
+#define _ABP2030PA_H
+
+#include <linux/types.h>
+
+#include <linux/iio/iio.h>
+
+#define ABP2_MEASUREMENT_RD_SIZE 7
+#define ABP2_CMD_NOP 0xf0
+#define ABP2_CMD_SYNC 0xaa
+#define ABP2_PKT_NOP_LEN ABP2_MEASUREMENT_RD_SIZE
+#define ABP2_PKT_SYNC_LEN 3
+
+struct completion;
+struct device;
+
+struct iio_chan_spec;
+struct iio_dev;
+
+struct abp2_data;
+struct abp2_ops;
+
+enum abp2_func_id {
+ ABP2_FUNCTION_A,
+};
+
+/**
+ * struct abp2_data
+ * @dev: current device structure
+ * @ops: pointers for bus specific read, write and init 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
+ * @buffer: 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 buffer[ABP2_MEASUREMENT_RD_SIZE] __aligned(IIO_DMA_MINALIGN);
+};
+
+struct abp2_ops {
+ int (*init)(struct device *dev);
+ int (*read)(struct abp2_data *data, const u8 cmd, const u8 cnt);
+ int (*write)(struct abp2_data *data, const u8 cmd, const u8 cnt);
+};
+
+int abp2_common_probe(struct device *dev, const struct abp2_ops *ops, int irq);
+
+#endif
diff --git a/drivers/iio/pressure/abp2030pa_i2c.c b/drivers/iio/pressure/abp2030pa_i2c.c
new file mode 100644
index 000000000000..34f365f452ef
--- /dev/null
+++ b/drivers/iio/pressure/abp2030pa_i2c.c
@@ -0,0 +1,93 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Honeywell ABP2 series pressure sensor driver
+ *
+ * Copyright (c) 2025 Petre Rodan <petre.rodan@subdimension.ro>
+ */
+
+#include <linux/device.h>
+#include <linux/errno.h>
+#include <linux/i2c.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/types.h>
+
+#include "abp2030pa.h"
+
+static int abp2_i2c_init(struct device *dev)
+{
+ return 0;
+}
+
+static int abp2_i2c_read(struct abp2_data *data, const u8 unused, const u8 cnt)
+{
+ struct i2c_client *client = to_i2c_client(data->dev);
+ int ret;
+
+ if (cnt > ABP2_MEASUREMENT_RD_SIZE)
+ return -EOVERFLOW;
+
+ ret = i2c_master_recv(client, data->buffer, cnt);
+ if (ret < 0)
+ return ret;
+ else if (ret != cnt)
+ return -EIO;
+
+ return 0;
+}
+
+static int abp2_i2c_write(struct abp2_data *data, const u8 cmd, const u8 unused)
+{
+ struct i2c_client *client = to_i2c_client(data->dev);
+ u8 wdata[ABP2_PKT_SYNC_LEN] = { cmd };
+ int ret;
+
+ ret = i2c_master_send(client, wdata, ABP2_PKT_SYNC_LEN);
+ if (ret < 0)
+ return ret;
+ else if (ret != ABP2_PKT_SYNC_LEN)
+ return -EIO;
+
+ return 0;
+}
+
+static const struct abp2_ops abp2_i2c_ops = {
+ .init = abp2_i2c_init,
+ .read = abp2_i2c_read,
+ .write = abp2_i2c_write,
+};
+
+static int abp2_i2c_probe(struct i2c_client *client)
+{
+ if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
+ return -EOPNOTSUPP;
+
+ return abp2_common_probe(&client->dev, &abp2_i2c_ops, client->irq);
+}
+
+static const struct of_device_id abp2_i2c_match[] = {
+ { .compatible = "honeywell,abp2030pa" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, abp2_i2c_match);
+
+static const struct i2c_device_id abp2_i2c_id[] = {
+ { "abp2030pa" },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, abp2_i2c_id);
+
+static struct i2c_driver abp2_i2c_driver = {
+ .driver = {
+ .name = "abp2030pa",
+ .of_match_table = abp2_i2c_match,
+ },
+ .probe = abp2_i2c_probe,
+ .id_table = abp2_i2c_id,
+};
+module_i2c_driver(abp2_i2c_driver);
+
+MODULE_AUTHOR("Petre Rodan <petre.rodan@subdimension.ro>");
+MODULE_DESCRIPTION("Honeywell ABP2 pressure sensor i2c driver");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS("IIO_HONEYWELL_ABP2030PA");
diff --git a/drivers/iio/pressure/abp2030pa_spi.c b/drivers/iio/pressure/abp2030pa_spi.c
new file mode 100644
index 000000000000..11a26f14d992
--- /dev/null
+++ b/drivers/iio/pressure/abp2030pa_spi.c
@@ -0,0 +1,88 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Honeywell ABP2 series pressure sensor driver
+ *
+ * Copyright (c) 2025 Petre Rodan <petre.rodan@subdimension.ro>
+ */
+
+#include <linux/device.h>
+#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"
+
+struct abp2_spi_buf {
+ u8 tx[ABP2_MEASUREMENT_RD_SIZE] __aligned(IIO_DMA_MINALIGN);
+};
+
+static int abp2_spi_init(struct device *dev)
+{
+ struct spi_device *spi = to_spi_device(dev);
+ struct abp2_spi_buf *buf;
+
+ buf = devm_kzalloc(dev, sizeof(*buf), GFP_KERNEL);
+ if (!buf)
+ return -ENOMEM;
+
+ spi_set_drvdata(spi, buf);
+
+ return 0;
+}
+
+static int abp2_spi_xfer(struct abp2_data *data, const u8 cmd, const u8 pkt_len)
+{
+ struct spi_device *spi = to_spi_device(data->dev);
+ struct abp2_spi_buf *buf = spi_get_drvdata(spi);
+ struct spi_transfer xfer;
+
+ if (pkt_len > ABP2_MEASUREMENT_RD_SIZE)
+ return -EOVERFLOW;
+
+ buf->tx[0] = cmd;
+ xfer.tx_buf = buf->tx;
+ xfer.rx_buf = data->buffer;
+ xfer.len = pkt_len;
+
+ return spi_sync_transfer(spi, &xfer, 1);
+}
+
+static const struct abp2_ops abp2_spi_ops = {
+ .init = abp2_spi_init,
+ .read = abp2_spi_xfer,
+ .write = abp2_spi_xfer,
+};
+
+static int abp2_spi_probe(struct spi_device *spi)
+{
+ return abp2_common_probe(&spi->dev, &abp2_spi_ops, spi->irq);
+}
+
+static const struct of_device_id abp2_spi_match[] = {
+ { .compatible = "honeywell,abp2030pa" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, abp2_spi_match);
+
+static const struct spi_device_id abp2_spi_id[] = {
+ { "abp2030pa" },
+ { }
+};
+MODULE_DEVICE_TABLE(spi, abp2_spi_id);
+
+static struct spi_driver abp2_spi_driver = {
+ .driver = {
+ .name = "abp2030pa",
+ .of_match_table = abp2_spi_match,
+ },
+ .probe = abp2_spi_probe,
+ .id_table = abp2_spi_id,
+};
+module_spi_driver(abp2_spi_driver);
+
+MODULE_AUTHOR("Petre Rodan <petre.rodan@subdimension.ro>");
+MODULE_DESCRIPTION("Honeywell ABP2 pressure sensor spi driver");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS("IIO_HONEYWELL_ABP2030PA");
--
2.51.0On Sat, Nov 22, 2025 at 11:42:45PM +0200, Petre Rodan 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 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 gpio can also be defined for the SPI variants if a
> non-ABP2 but compatible chip is to be driven.
>
> Tested on two different sensors.
> 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@subdimension.ro>
Make it Datasheet tag:
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@subdimension.ro>
...
> +config ABP2030PA
> + tristate "Honeywell ABP2 pressure sensor series"
> + depends on (I2C || SPI_MASTER)
> + select ABP2030PA_I2C if I2C
> + select ABP2030PA_SPI if SPI_MASTER
> + select IIO_BUFFER
> + select IIO_TRIGGERED_BUFFER
> + help
> + Say Y here to build support for the Honeywell ABP2 board mount
> + pressure and temperature sensor series.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called abp2030pa.
> +
> +config ABP2030PA_I2C
> + tristate
> + depends on ABP2030PA
> + depends on I2C
> +
> +config ABP2030PA_SPI
> + tristate
> + depends on ABP2030PA
> + depends on SPI_MASTER
Please, do it other way around, i.e. let user define if they want or not to
have the respective glue driver and select the core from each.
...
> +/*
> + * Honeywell ABP2 series pressure sensor driver
> + *
> + * Copyright (c) 2025 Petre Rodan <petre.rodan@subdimension.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
> + *
Redundant blank line.
> + */
> +
> +#include <linux/array_size.h>
+ bits.h
+ completion.h ?
> +#include <linux/delay.h>
+ dev_printk.h
+ device.h
+ errno.h
+ export.h
> +#include <linux/gpio/consumer.h>
+ interrupt.h
> +#include <linux/math64.h>
> +#include <linux/module.h>
> +#include <linux/property.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +#include <linux/units.h>
> +
Redundant blank line.
> +#include <linux/unaligned.h>
...
> +/*
> + * transfer function A: 10% to 90% of 2^24
Too many spaces, also this may be a one-line comment.
> + */
> +static const struct abp2_func_spec abp2_func_spec[] = {
> + [ABP2_FUNCTION_A] = { .output_min = 1677722, .output_max = 15099494 },
> +};
...
> +enum abp2_variants {
Why explicit assignments? Is it related to some HW register?
> + ABP2001BA = 0x0, ABP21_6BA = 0x1, ABP22_5BA = 0x2, ABP2004BA = 0x3,
> + ABP2006BA = 0x4, ABP2008BA = 0x5, ABP2010BA = 0x6, ABP2012BA = 0x7,
> + ABP2001BD = 0x8, ABP21_6BD = 0x9, ABP22_5BD = 0xa, ABP2004BD = 0xb,
> + ABP2001BG = 0xc, ABP21_6BG = 0xd, ABP22_5BG = 0xe, ABP2004BG = 0xf,
> + ABP2006BG = 0x10, ABP2008BG = 0x11, ABP2010BG = 0x12, ABP2012BG = 0x13,
> + ABP2001GG = 0x14, ABP21_2GG = 0x15, ABP2100KA = 0x16, ABP2160KA = 0x17,
> + ABP2250KA = 0x18, ABP2001KD = 0x19, ABP21_6KD = 0x1a, ABP22_5KD = 0x1b,
> + ABP2004KD = 0x1c, ABP2006KD = 0x1d, ABP2010KD = 0x1e, ABP2016KD = 0x1f,
> + ABP2025KD = 0x20, ABP2040KD = 0x21, ABP2060KD = 0x22, ABP2100KD = 0x23,
> + ABP2160KD = 0x24, ABP2250KD = 0x25, ABP2400KD = 0x26, ABP2001KG = 0x27,
> + ABP21_6KG = 0x28, ABP22_5KG = 0x29, ABP2004KG = 0x2a, ABP2006KG = 0x2b,
> + ABP2010KG = 0x2c, ABP2016KG = 0x2d, ABP2025KG = 0x2e, ABP2040KG = 0x2f,
> + ABP2060KG = 0x30, ABP2100KG = 0x31, ABP2160KG = 0x32, ABP2250KG = 0x33,
> + ABP2400KG = 0x34, ABP2600KG = 0x35, ABP2800KG = 0x36, ABP2250LD = 0x37,
> + ABP2600LD = 0x38, ABP2600LG = 0x39, ABP22_5MD = 0x3a, ABP2006MD = 0x3b,
> + ABP2010MD = 0x3c, ABP2016MD = 0x3d, ABP2025MD = 0x3e, ABP2040MD = 0x3f,
> + ABP2060MD = 0x40, ABP2100MD = 0x41, ABP2160MD = 0x42, ABP2250MD = 0x43,
> + ABP2400MD = 0x44, ABP2600MD = 0x45, ABP2006MG = 0x46, ABP2010MG = 0x47,
> + ABP2016MG = 0x48, ABP2025MG = 0x49, ABP2040MG = 0x4a, ABP2060MG = 0x4b,
> + ABP2100MG = 0x4c, ABP2160MG = 0x4d, ABP2250MG = 0x4e, ABP2400MG = 0x4f,
> + ABP2600MG = 0x50, ABP2001ND = 0x51, ABP2002ND = 0x52, ABP2004ND = 0x53,
> + ABP2005ND = 0x54, ABP2010ND = 0x55, ABP2020ND = 0x56, ABP2030ND = 0x57,
> + ABP2002NG = 0x58, ABP2004NG = 0x59, ABP2005NG = 0x5a, ABP2010NG = 0x5b,
> + ABP2020NG = 0x5c, ABP2030NG = 0x5d, ABP2015PA = 0x5e, ABP2030PA = 0x5f,
> + ABP2060PA = 0x60, ABP2100PA = 0x61, ABP2150PA = 0x62, ABP2175PA = 0x63,
> + ABP2001PD = 0x64, ABP2005PD = 0x65, ABP2015PD = 0x66, ABP2030PD = 0x67,
> + ABP2060PD = 0x68, ABP2001PG = 0x69, ABP2005PG = 0x6a, ABP2015PG = 0x6b,
> + ABP2030PG = 0x6c, ABP2060PG = 0x6d, ABP2100PG = 0x6e, ABP2150PG = 0x6f,
> + ABP2175PG = 0x70, ABP2_VARIANTS_MAX
If required at all, make sure _MAX occupies a full line:
ABP2175PG,
ABP2_VARIANTS_MAX
This allow to avoid unnecessary churn in the future in case of changes here.
> +};
...
> +static const char * const abp2_triplet_variants[ABP2_VARIANTS_MAX] = {
> + [ABP2001BA] = "001BA", [ABP21_6BA] = "1.6BA", [ABP22_5BA] = "2.5BA",
> + [ABP2004BA] = "004BA", [ABP2006BA] = "006BA", [ABP2008BA] = "008BA",
> + [ABP2010BA] = "010BA", [ABP2012BA] = "012BA", [ABP2001BD] = "001BD",
> + [ABP21_6BD] = "1.6BD", [ABP22_5BD] = "2.5BD", [ABP2004BD] = "004BD",
> + [ABP2001BG] = "001BG", [ABP21_6BG] = "1.6BG", [ABP22_5BG] = "2.5BG",
> + [ABP2004BG] = "004BG", [ABP2006BG] = "006BG", [ABP2008BG] = "008BG",
> + [ABP2010BG] = "010BG", [ABP2012BG] = "012BG", [ABP2001GG] = "001GG",
> + [ABP21_2GG] = "1.2GG", [ABP2100KA] = "100KA", [ABP2160KA] = "160KA",
> + [ABP2250KA] = "250KA", [ABP2001KD] = "001KD", [ABP21_6KD] = "1.6KD",
> + [ABP22_5KD] = "2.5KD", [ABP2004KD] = "004KD", [ABP2006KD] = "006KD",
> + [ABP2010KD] = "010KD", [ABP2016KD] = "016KD", [ABP2025KD] = "025KD",
> + [ABP2040KD] = "040KD", [ABP2060KD] = "060KD", [ABP2100KD] = "100KD",
> + [ABP2160KD] = "160KD", [ABP2250KD] = "250KD", [ABP2400KD] = "400KD",
> + [ABP2001KG] = "001KG", [ABP21_6KG] = "1.6KG", [ABP22_5KG] = "2.5KG",
> + [ABP2004KG] = "004KG", [ABP2006KG] = "006KG", [ABP2010KG] = "010KG",
> + [ABP2016KG] = "016KG", [ABP2025KG] = "025KG", [ABP2040KG] = "040KG",
> + [ABP2060KG] = "060KG", [ABP2100KG] = "100KG", [ABP2160KG] = "160KG",
> + [ABP2250KG] = "250KG", [ABP2400KG] = "400KG", [ABP2600KG] = "600KG",
> + [ABP2800KG] = "800KG", [ABP2250LD] = "250LD", [ABP2600LD] = "600LD",
> + [ABP2600LG] = "600LG", [ABP22_5MD] = "2.5MD", [ABP2006MD] = "006MD",
> + [ABP2010MD] = "010MD", [ABP2016MD] = "016MD", [ABP2025MD] = "025MD",
> + [ABP2040MD] = "040MD", [ABP2060MD] = "060MD", [ABP2100MD] = "100MD",
> + [ABP2160MD] = "160MD", [ABP2250MD] = "250MD", [ABP2400MD] = "400MD",
> + [ABP2600MD] = "600MD", [ABP2006MG] = "006MG", [ABP2010MG] = "010MG",
> + [ABP2016MG] = "016MG", [ABP2025MG] = "025MG", [ABP2040MG] = "040MG",
> + [ABP2060MG] = "060MG", [ABP2100MG] = "100MG", [ABP2160MG] = "160MG",
> + [ABP2250MG] = "250MG", [ABP2400MG] = "400MG", [ABP2600MG] = "600MG",
> + [ABP2001ND] = "001ND", [ABP2002ND] = "002ND", [ABP2004ND] = "004ND",
> + [ABP2005ND] = "005ND", [ABP2010ND] = "010ND", [ABP2020ND] = "020ND",
> + [ABP2030ND] = "030ND", [ABP2002NG] = "002NG", [ABP2004NG] = "004NG",
> + [ABP2005NG] = "005NG", [ABP2010NG] = "010NG", [ABP2020NG] = "020NG",
> + [ABP2030NG] = "030NG", [ABP2015PA] = "015PA", [ABP2030PA] = "030PA",
> + [ABP2060PA] = "060PA", [ABP2100PA] = "100PA", [ABP2150PA] = "150PA",
> + [ABP2175PA] = "175PA", [ABP2001PD] = "001PD", [ABP2005PD] = "005PD",
> + [ABP2015PD] = "015PD", [ABP2030PD] = "030PD", [ABP2060PD] = "060PD",
> + [ABP2001PG] = "001PG", [ABP2005PG] = "005PG", [ABP2015PG] = "015PG",
> + [ABP2030PG] = "030PG", [ABP2060PG] = "060PG", [ABP2100PG] = "100PG",
> + [ABP2150PG] = "150PG", [ABP2175PG] = "175PG"
Leave trailing comma in place.
> +};
Can be done easier with a macro with more robustness against typos:
#define ABP2_VARIANT(v) [ABP2 ## v] = ##v
static const char * const abp2_triplet_variants[] = {
ABP2_VARIANT(001BA), ABP2_VARIANT(1_6BA), ABP2_VARIANT(2_5BA), ABP2_VARIANT(004BA),
...
};
but this will loose the possibility to make '.' in the name. Up to you.
> +/**
> + * struct abp2_range_config - list of pressure ranges based on nomenclature
> + * @pmin: lowest pressure that can be measured
> + * @pmax: highest pressure that can be measured
> + */
> +struct abp2_range_config {
> + const s32 pmin;
> + const s32 pmax;
What is the meaning of 'const' here, please?
> +};
...
> +static int abp2_get_measurement(struct abp2_data *data)
> +{
> + struct device *dev = data->dev;
> + int ret;
> +
> + memset(data->buffer, 0, sizeof(data->buffer));
> + reinit_completion(&data->completion);
> +
> + ret = data->ops->write(data, ABP2_CMD_SYNC, ABP2_PKT_SYNC_LEN);
> + if (ret < 0)
> + return ret;
> +
> + if (data->irq > 0) {
> + ret = wait_for_completion_timeout(&data->completion, HZ);
Where is HZ defined? Include that.
> + if (!ret) {
> + dev_err(dev, "timeout waiting for EOC interrupt\n");
> + return -ETIMEDOUT;
> + }
> + } else
> + fsleep(5000);
Better to have 5 * USEC_PER_MSEC. Also missed comment why this long delay
is needed (will require time.h).
Missed {} as well.
> + ret = data->ops->read(data, ABP2_CMD_NOP, ABP2_PKT_NOP_LEN);
> + if (ret < 0)
> + return ret;
> +
> + /*
> + * status byte flags
> + * bit7 SANITY_CHK - must always be 0
> + * bit6 ABP2_ST_POWER - 1 if device is powered
> + * bit5 ABP2_ST_BUSY - 1 if device has no new conversion ready
> + * bit4 SANITY_CHK - must always be 0
> + * bit3 SANITY_CHK - must always be 0
> + * bit2 MEMORY_ERR - 1 if integrity test has failed
> + * bit1 SANITY_CHK - must always be 0
> + * bit0 MATH_ERR - 1 during internal math saturation err
> + */
> +
> + if (data->buffer[0] == (ABP2_ST_POWER | ABP2_ST_BUSY))
> + return -ETIMEDOUT;
> +
> + if (data->buffer[0] != ABP2_ST_POWER) {
> + dev_err(data->dev,
> + "unexpected status byte 0x%02x\n", data->buffer[0]);
> + return -ETIMEDOUT;
> + }
I am not sure the chosen error code in both cases is good enough.
> + return 0;
> +}
...
> +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;
> + s64 tmp;
> +
> + 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);
> + indio_dev->modes = INDIO_DIRECT_MODE;
Doesn't IIO core take care of this?
> + ret = devm_regulator_get_enable(dev, "vdd");
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "can't get and enable vdd supply\n");
> +
> + ret = data->ops->init(data->dev);
> + if (ret)
> + return ret;
> +
> + ret = device_property_read_string(dev, "honeywell,pressure-triplet",
> + &triplet);
> + if (ret) {
> + ret = device_property_read_u32(dev, "honeywell,pmin-pascal",
> + &data->pmin);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "honeywell,pmin-pascal could not be read\n");
> +
> + ret = device_property_read_u32(dev, "honeywell,pmax-pascal",
> + &data->pmax);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "honeywell,pmax-pascal could not be read\n");
> + } else {
> + ret = device_property_match_property_string(dev,
> + "honeywell,pressure-triplet",
> + abp2_triplet_variants,
> + ABP2_VARIANTS_MAX);
> + if (ret < 0)
> + return dev_err_probe(dev, -EINVAL,
> + "honeywell,pressure-triplet is invalid\n");
> +
> + data->pmin = abp2_range_config[ret].pmin;
> + data->pmax = abp2_range_config[ret].pmax;
> + }
> +
> + if (data->pmin >= data->pmax)
> + return dev_err_probe(dev, -EINVAL,
> + "pressure limits are invalid\n");
> +
> + data->outmin = abp2_func_spec[data->function].output_min;
> + data->outmax = abp2_func_spec[data->function].output_max;
> +
> + tmp = div_s64(((s64)(data->pmax - data->pmin)) * NANO,
> + data->outmax - data->outmin);
> + data->p_scale = div_s64_rem(tmp, NANO, &data->p_scale_dec);
> +
> + tmp = div_s64((s64)data->pmin * (s64)(data->outmax - data->outmin),
> + (s64)(data->pmax - data->pmin)) - (s64)(data->outmin);
> + data->p_offset = tmp;
> +
> + if (data->irq > 0) {
> + ret = devm_request_irq(dev, data->irq, abp2_eoc_handler,
> + IRQF_TRIGGER_RISING,
> + dev_name(dev),
> + data);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "request irq %d failed\n", data->irq);
> + }
> +
> + ret = devm_iio_triggered_buffer_setup(dev, indio_dev, NULL,
> + abp2_trigger_handler, NULL);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "iio triggered buffer setup failed\n");
> +
> + ret = devm_iio_device_register(dev, indio_dev);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "unable to register iio device\n");
> +
> + return 0;
> +}
...
> +#ifndef _ABP2030PA_H
> +#define _ABP2030PA_H
> +
> +#include <linux/types.h>
> +
> +#include <linux/iio/iio.h>
> +
> +struct completion;
No, you need a full header because it's not a pointer.
> +struct device;
> +struct iio_chan_spec;
> +struct iio_dev;
When iio/iio.h is included, do we need these two?
> +struct abp2_data;
> +struct abp2_ops;
> +
> +enum abp2_func_id {
> + ABP2_FUNCTION_A,
> +};
> +
> +/**
> + * struct abp2_data
> + * @dev: current device structure
> + * @ops: pointers for bus specific read, write and init 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
> + * @buffer: 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 buffer[ABP2_MEASUREMENT_RD_SIZE] __aligned(IIO_DMA_MINALIGN);
> +};
> +struct abp2_ops {
> + int (*init)(struct device *dev);
> + int (*read)(struct abp2_data *data, const u8 cmd, const u8 cnt);
> + int (*write)(struct abp2_data *data, const u8 cmd, const u8 cnt);
What is the meaning of const for the POD type parameters? I mean this gives
really a little protection if any. I do not see a point here to have them being const.
> +};
> +int abp2_common_probe(struct device *dev, const struct abp2_ops *ops, int irq);
> +
> +#endif
...
> +#include <linux/device.h>
> +#include <linux/errno.h>
> +#include <linux/i2c.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/types.h>
> +static int abp2_i2c_init(struct device *dev)
> +{
> + return 0;
> +}
Is this stub required?
...
> +static int abp2_i2c_read(struct abp2_data *data, const u8 unused, const u8 cnt)
> +{
> + struct i2c_client *client = to_i2c_client(data->dev);
> + int ret;
> +
> + if (cnt > ABP2_MEASUREMENT_RD_SIZE)
> + return -EOVERFLOW;
> +
> + ret = i2c_master_recv(client, data->buffer, cnt);
> + if (ret < 0)
> + return ret;
> + else if (ret != cnt)
Redundant 'else'.
> + return -EIO;
> +
> + return 0;
> +}
> +
> +static int abp2_i2c_write(struct abp2_data *data, const u8 cmd, const u8 unused)
> +{
> + struct i2c_client *client = to_i2c_client(data->dev);
> + u8 wdata[ABP2_PKT_SYNC_LEN] = { cmd };
> + int ret;
> +
> + ret = i2c_master_send(client, wdata, ABP2_PKT_SYNC_LEN);
> + if (ret < 0)
> + return ret;
> + else if (ret != ABP2_PKT_SYNC_LEN)
> + return -EIO;
Ditto.
> + return 0;
> +}
> +
> +static const struct abp2_ops abp2_i2c_ops = {
> + .init = abp2_i2c_init,
> + .read = abp2_i2c_read,
> + .write = abp2_i2c_write,
> +};
So, why can't regmap I²C be used?
...
> +#include <linux/device.h>
Is it used? Or only device/devres.h?
> +#include <linux/errno.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/spi/spi.h>
> +#include <linux/types.h>
...
> +static int abp2_spi_init(struct device *dev)
> +{
> + struct spi_device *spi = to_spi_device(dev);
> + struct abp2_spi_buf *buf;
> +
> + buf = devm_kzalloc(dev, sizeof(*buf), GFP_KERNEL);
Using devm_*() here is most likely a mistake. What is the object lifetime in
comparison to the used device?
> + if (!buf)
> + return -ENOMEM;
> +
> + spi_set_drvdata(spi, buf);
> +
> + return 0;
> +}
...
> +static int abp2_spi_xfer(struct abp2_data *data, const u8 cmd, const u8 pkt_len)
> +{
> + struct spi_device *spi = to_spi_device(data->dev);
> + struct abp2_spi_buf *buf = spi_get_drvdata(spi);
> + struct spi_transfer xfer;
> +
> + if (pkt_len > ABP2_MEASUREMENT_RD_SIZE)
> + return -EOVERFLOW;
> +
> + buf->tx[0] = cmd;
> + xfer.tx_buf = buf->tx;
> + xfer.rx_buf = data->buffer;
> + xfer.len = pkt_len;
> +
> + return spi_sync_transfer(spi, &xfer, 1);
> +}
> +
> +static const struct abp2_ops abp2_spi_ops = {
> + .init = abp2_spi_init,
> + .read = abp2_spi_xfer,
> + .write = abp2_spi_xfer,
> +};
Why not regmap SPI?
--
With Best Regards,
Andy Shevchenko
hello Andy.
thank you for the review.
On Mon, Nov 24, 2025 at 01:48:08PM +0200, Andy Shevchenko wrote:
> On Sat, Nov 22, 2025 at 11:42:45PM +0200, Petre Rodan wrote:
[..]
> > +/*
> > + * transfer function A: 10% to 90% of 2^24
>
> Too many spaces, also this may be a one-line comment.
it was intentional to have the comment multiline.
in case we need to add additional transfer functions in the future for compatible ICs the diff will be a few lines smaller.
> > + */
> > +static const struct abp2_func_spec abp2_func_spec[] = {
> > + [ABP2_FUNCTION_A] = { .output_min = 1677722, .output_max = 15099494 },
> > +};
>
> ...
>
> > +enum abp2_variants {
>
> Why explicit assignments? Is it related to some HW register?
no registers, I just need to ensure the two arrays
static const char * const abp2_triplet_variants[ABP2_VARIANTS_MAX] = {
[ABP2001BA] = "001BA", [ABP21_6BA] = "1.6BA", [ABP22_5BA] = "2.5BA", ..
static const struct abp2_range_config abp2_range_config[ABP2_VARIANTS_MAX] = {
[ABP2001BA] = { .pmin = 0, .pmax = 100000 },
[ABP21_6BA] = { .pmin = 0, .pmax = 160000 }, ..
keep being consistent and are resistant to later editing.
I feel like I had a better implementation two years ago when I used a single struct containing all this information and had a custom/NIH search function, but you kindly asked me [1] to use device_property_match_property_string() instead and split my single struct into this three parts mess.
[1] https://lore.kernel.org/linux-iio/ZWcUPkzfGqxYsysp@smile.fi.intel.com/
> Can be done easier with a macro with more robustness against typos:
>
> #define ABP2_VARIANT(v) [ABP2 ## v] = ##v
>
> static const char * const abp2_triplet_variants[] = {
> ABP2_VARIANT(001BA), ABP2_VARIANT(1_6BA), ABP2_VARIANT(2_5BA), ABP2_VARIANT(004BA),
> ...
> };
>
> but this will loose the possibility to make '.' in the name. Up to you.
thanks, but I need '.' in the name. the dot is used in the part number (and thus in the pressure triplet).
> > +static int abp2_get_measurement(struct abp2_data *data)
> > +{
> > + struct device *dev = data->dev;
> > + int ret;
> > +
> > + memset(data->buffer, 0, sizeof(data->buffer));
> > + reinit_completion(&data->completion);
> > +
> > + ret = data->ops->write(data, ABP2_CMD_SYNC, ABP2_PKT_SYNC_LEN);
> > + if (ret < 0)
> > + return ret;
> > +
> > + if (data->irq > 0) {
> > + ret = wait_for_completion_timeout(&data->completion, HZ);
>
> Where is HZ defined? Include that.
>
> > + if (!ret) {
> > + dev_err(dev, "timeout waiting for EOC interrupt\n");
> > + return -ETIMEDOUT;
> > + }
> > + } else
> > + fsleep(5000);
>
> Better to have 5 * USEC_PER_MSEC. Also missed comment why this long delay
> is needed (will require time.h).
>
> Missed {} as well.
I'm not sure I understand where are braces needed/not needed in this context.
> > + ret = data->ops->read(data, ABP2_CMD_NOP, ABP2_PKT_NOP_LEN);
> > + if (ret < 0)
> > + return ret;
> > +
> > + /*
> > + * status byte flags
> > + * bit7 SANITY_CHK - must always be 0
> > + * bit6 ABP2_ST_POWER - 1 if device is powered
> > + * bit5 ABP2_ST_BUSY - 1 if device has no new conversion ready
> > + * bit4 SANITY_CHK - must always be 0
> > + * bit3 SANITY_CHK - must always be 0
> > + * bit2 MEMORY_ERR - 1 if integrity test has failed
> > + * bit1 SANITY_CHK - must always be 0
> > + * bit0 MATH_ERR - 1 during internal math saturation err
> > + */
> > +
> > + if (data->buffer[0] == (ABP2_ST_POWER | ABP2_ST_BUSY))
> > + return -ETIMEDOUT;
> > +
> > + if (data->buffer[0] != ABP2_ST_POWER) {
> > + dev_err(data->dev,
> > + "unexpected status byte 0x%02x\n", data->buffer[0]);
> > + return -ETIMEDOUT;
> > + }
>
> I am not sure the chosen error code in both cases is good enough.
I'm open to recommendations on better error codes.
first error: chip reports it's busy 5ms after start conversion command. based on the datasheet the conversion should have been ready at this point in time. this sounds to me like a timeout error.
second error: status byte contains unexpected flags being set - either an internal error - see table above or a bus read error. yes, timeout is not good here but what should it be?
I'm using two conditionals because I want to log only invalid statuses and ignore simple 'device busy' errors.
> > +struct abp2_ops {
> > + int (*init)(struct device *dev);
> > + int (*read)(struct abp2_data *data, const u8 cmd, const u8 cnt);
> > + int (*write)(struct abp2_data *data, const u8 cmd, const u8 cnt);
>
> What is the meaning of const for the POD type parameters? I mean this gives
> really a little protection if any. I do not see a point here to have them being const.
I read a few books about C programming a few decades back and there was a consensus on using const in function prototypes wherever a parameter was supposed to not be changed.
of course it's not bulletproof, but why do you feel I should stop following that advice for functions that are not tied to any pre-existing kernel API?
> > +int abp2_common_probe(struct device *dev, const struct abp2_ops *ops, int irq);
> > +
> > +#endif
>
> ...
>
> > +#include <linux/device.h>
> > +#include <linux/errno.h>
> > +#include <linux/i2c.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/types.h>
>
> > +static int abp2_i2c_init(struct device *dev)
> > +{
> > + return 0;
> > +}
>
> Is this stub required?
do I have a 100% guarantee that the kernel will not try to execute a null pointer function in abp2_common_probe()?
ret = data->ops->init(data->dev); // needed only for SPI.
later edit:
since I will remove devm_kzalloc(), the _init will probably go away entirely together with the stub.
> > +static int abp2_i2c_read(struct abp2_data *data, const u8 unused, const u8 cnt)
> > +{
> > + struct i2c_client *client = to_i2c_client(data->dev);
> > + int ret;
> > +
> > + if (cnt > ABP2_MEASUREMENT_RD_SIZE)
> > + return -EOVERFLOW;
> > +
> > + ret = i2c_master_recv(client, data->buffer, cnt);
> > + if (ret < 0)
> > + return ret;
>
> > + else if (ret != cnt)
>
> Redundant 'else'.
>
> > + return -EIO;
> > +
> > + return 0;
> > +}
are you implying that __i2c_transfer() errors out if the number of bytes transfered is not cnt?
> > +static const struct abp2_ops abp2_i2c_ops = {
> > + .init = abp2_i2c_init,
> > + .read = abp2_i2c_read,
> > + .write = abp2_i2c_write,
> > +};
>
> So, why can't regmap I²C be used?
[..]
> So, why can't regmap SPI be used?
there are no registers, no memory map, just a 'start conversion' and the equivalent of a 'read conversion' command.
any reason one would use the regmap API in this case?
best regards,
peter
On Mon, Nov 24, 2025 at 07:29:06PM +0200, Petre Rodan wrote:
> thank you for the review.
You're welcome.
> On Mon, Nov 24, 2025 at 01:48:08PM +0200, Andy Shevchenko wrote:
> > On Sat, Nov 22, 2025 at 11:42:45PM +0200, Petre Rodan wrote:
[..]
> > > +/*
> > > + * transfer function A: 10% to 90% of 2^24
> >
> > Too many spaces, also this may be a one-line comment.
>
> it was intentional to have the comment multiline.
> in case we need to add additional transfer functions in the future for
> compatible ICs the diff will be a few lines smaller.
This is just a comment, won't be a big churn to change (note, it's different
to the cases of trailing commas where it has more significant impact).
> > > + */
...
> > > +enum abp2_variants {
> >
> > Why explicit assignments? Is it related to some HW register?
>
> no registers, I just need to ensure the two arrays
>
> static const char * const abp2_triplet_variants[ABP2_VARIANTS_MAX] = {
> [ABP2001BA] = "001BA", [ABP21_6BA] = "1.6BA", [ABP22_5BA] = "2.5BA", ..
>
> static const struct abp2_range_config abp2_range_config[ABP2_VARIANTS_MAX] = {
> [ABP2001BA] = { .pmin = 0, .pmax = 100000 },
> [ABP21_6BA] = { .pmin = 0, .pmax = 160000 }, ..
>
> keep being consistent and are resistant to later editing.
So, if it's pure software numbering, just drop assignments in the enum.
> I feel like I had a better implementation two years ago when I used a single
> struct containing all this information and had a custom/NIH search function,
> but you kindly asked me [1] to use device_property_match_property_string()
> instead and split my single struct into this three parts mess.
Yes, and that still stays.
> [1] https://lore.kernel.org/linux-iio/ZWcUPkzfGqxYsysp@smile.fi.intel.com/
> > Can be done easier with a macro with more robustness against typos:
> >
> > #define ABP2_VARIANT(v) [ABP2 ## v] = ##v
> >
> > static const char * const abp2_triplet_variants[] = {
> > ABP2_VARIANT(001BA), ABP2_VARIANT(1_6BA), ABP2_VARIANT(2_5BA), ABP2_VARIANT(004BA),
> > ...
> > };
> >
> > but this will loose the possibility to make '.' in the name. Up to you.
>
> thanks, but I need '.' in the name. the dot is used in the part number (and
> thus in the pressure triplet).
OK.
...
> > > + if (data->irq > 0) {
> > > + ret = wait_for_completion_timeout(&data->completion, HZ);
> >
> > Where is HZ defined? Include that.
> >
> > > + if (!ret) {
> > > + dev_err(dev, "timeout waiting for EOC interrupt\n");
> > > + return -ETIMEDOUT;
> > > + }
> > > + } else
> > > + fsleep(5000);
> >
> > Better to have 5 * USEC_PER_MSEC. Also missed comment why this long delay
> > is needed (will require time.h).
> >
> > Missed {} as well.
>
> I'm not sure I understand where are braces needed/not needed in this context.
It's according to the Coding Style. If one branch needs that, the second must
be on par.
...
> > > + /*
> > > + * status byte flags
> > > + * bit7 SANITY_CHK - must always be 0
> > > + * bit6 ABP2_ST_POWER - 1 if device is powered
> > > + * bit5 ABP2_ST_BUSY - 1 if device has no new conversion ready
> > > + * bit4 SANITY_CHK - must always be 0
> > > + * bit3 SANITY_CHK - must always be 0
> > > + * bit2 MEMORY_ERR - 1 if integrity test has failed
> > > + * bit1 SANITY_CHK - must always be 0
> > > + * bit0 MATH_ERR - 1 during internal math saturation err
> > > + */
> > > +
> > > + if (data->buffer[0] == (ABP2_ST_POWER | ABP2_ST_BUSY))
> > > + return -ETIMEDOUT;
-EBUSY as I read the comment above.
> > > + if (data->buffer[0] != ABP2_ST_POWER) {
> > > + dev_err(data->dev,
> > > + "unexpected status byte 0x%02x\n", data->buffer[0]);
> > > + return -ETIMEDOUT;
-EIO
> > > + }
> >
> > I am not sure the chosen error code in both cases is good enough.
>
> I'm open to recommendations on better error codes.
See below.
> first error: chip reports it's busy 5ms after start conversion command. based
> on the datasheet the conversion should have been ready at this point in time.
> this sounds to me like a timeout error.
I think EBUSY consistent with the comment given above in the code.
> second error: status byte contains unexpected flags being set - either an
> internal error - see table above or a bus read error. yes, timeout is not
> good here but what should it be?
>
> I'm using two conditionals because I want to log only invalid statuses and
> ignore simple 'device busy' errors.
You know the HW much better than me, so I proposed on the context I see in the
code, you may take the advice or do differently, but -ETIMEDOUT in these cases
seems to me not a fit.
...
> > > +struct abp2_ops {
> > > + int (*init)(struct device *dev);
> > > + int (*read)(struct abp2_data *data, const u8 cmd, const u8 cnt);
> > > + int (*write)(struct abp2_data *data, const u8 cmd, const u8 cnt);
> >
> > What is the meaning of const for the POD type parameters? I mean this gives
> > really a little protection if any. I do not see a point here to have them being const.
>
> I read a few books about C programming a few decades back and there was a
> consensus on using const in function prototypes wherever a parameter was
> supposed to not be changed. of course it's not bulletproof, but why do you
> feel I should stop following that advice for functions that are not tied to
> any pre-existing kernel API?
This is quite good advice for the pointers to the arrays and / or structures,
but for simple numbers it makes a little sense inside one small driver.
(Although, it has more sense in the generic libraries, but still.)
> > > +int abp2_common_probe(struct device *dev, const struct abp2_ops *ops, int irq);
You see, here I haven't commented anything as const qualifier for ops here is
very good and should be present like you have done already.
> > > +#endif
...
> > > +static int abp2_i2c_init(struct device *dev)
> > > +{
> > > + return 0;
> > > +}
> >
> > Is this stub required?
>
> do I have a 100% guarantee that the kernel will not try to execute a null
> pointer function in abp2_common_probe()?
Depends on what you do with this. If you do not check at the caller, then yes,
stub is needed, but in kernel we usually do
if (...->init) {
ret = ->init(...);
...
}
> ret = data->ops->init(data->dev); // needed only for SPI.
>
> later edit:
> since I will remove devm_kzalloc(), the _init will probably go away entirely
> together with the stub.
OK.
...
> > > +static int abp2_i2c_read(struct abp2_data *data, const u8 unused, const u8 cnt)
> > > +{
> > > + struct i2c_client *client = to_i2c_client(data->dev);
> > > + int ret;
> > > +
> > > + if (cnt > ABP2_MEASUREMENT_RD_SIZE)
> > > + return -EOVERFLOW;
> > > +
> > > + ret = i2c_master_recv(client, data->buffer, cnt);
> > > + if (ret < 0)
> > > + return ret;
> >
> > > + else if (ret != cnt)
> >
> > Redundant 'else'.
> > > + return -EIO;
> > > +
> > > + return 0;
> > > +}
>
> are you implying that __i2c_transfer() errors out if the number of bytes transfered is not cnt?
git log --grep "redundant 'else'"
will answer to this question.
[..]
> > So, why can't regmap SPI be used?
>
> there are no registers, no memory map, just a 'start conversion' and the
> equivalent of a 'read conversion' command.
> any reason one would use the regmap API in this case?
At bare minimum the commit message should have a justification for the choice
explaining all this. Ideally, try to find a way how to use regmap API. We have
several weeks of time for this exercise.
--
With Best Regards,
Andy Shevchenko
hello!
On Mon, Nov 24, 2025 at 08:41:32PM +0200, Andy Shevchenko wrote:
> On Mon, Nov 24, 2025 at 07:29:06PM +0200, Petre Rodan wrote:
>
> > > Why explicit assignments? Is it related to some HW register?
> >
> > no registers, I just need to ensure the two arrays
> >
> > static const char * const abp2_triplet_variants[ABP2_VARIANTS_MAX] = {
> > [ABP2001BA] = "001BA", [ABP21_6BA] = "1.6BA", [ABP22_5BA] = "2.5BA", ..
> >
> > static const struct abp2_range_config abp2_range_config[ABP2_VARIANTS_MAX] = {
> > [ABP2001BA] = { .pmin = 0, .pmax = 100000 },
> > [ABP21_6BA] = { .pmin = 0, .pmax = 160000 }, ..
> >
> > keep being consistent and are resistant to later editing.
>
> So, if it's pure software numbering, just drop assignments in the enum.
so you want the consistency check to be dropped? we have data in two different arrays and they must be kept in perfect sync. if I were to remove the assignments someone comes a few years in the future, inserts a new device in the abp2_triplet_variants array at position 84 out of 113, also inserts a new {pmin, pmax} into the abp2_range_config array accidentally at position 83 and the compiler will be none the wiser.
just the day before I had to remove a variant because of a typo in the datasheet. I cheat and use a script to generate the structs [1], but if I had to modify them by hand, the assignments would make sure I delete the proper line.
am I proud of this? no, and I told you my preference. this is just a compromise that uses the non-specific match function and still provides a guardrail for future editing.
[1] https://codeberg.org/subDIMENSION/lkm_sandbox/src/branch/main/honeywell_abp2030pa/scripts/parse_variants_table.sh
[..]
> > > So, why can't regmap SPI be used?
> >
> > there are no registers, no memory map, just a 'start conversion' and the
> > equivalent of a 'read conversion' command.
> > any reason one would use the regmap API in this case?
>
> At bare minimum the commit message should have a justification for the choice
> explaining all this.
I had the justification in the cover letter instead, my bad, will include it in
the commit message instead.
> Ideally, try to find a way how to use regmap API. We have several weeks of
> time for this exercise.
you did not mention why use an API designed for devices with registers and a memory map on an IC that has neither.
I also have a bughunt in the spi-omap2-mcspi driver related to improper CS delays in queued transfers, regmap will probably just be an extra layer of abstraction I will have to go thru :/
[..]
oh, and
struct abp2_spi_buf {
u8 tx[ABP2_MEASUREMENT_RD_SIZE] __aligned(IIO_DMA_MINALIGN);
};
static int abp2_spi_init(struct device *dev)
{
struct spi_device *spi = to_spi_device(dev);
struct abp2_spi_buf *buf;
buf = devm_kzalloc(dev, sizeof(*buf), GFP_KERNEL);
> Using devm_*() here is most likely a mistake. What is the object lifetime in
> comparison to the used device?
I did think that placing this into the abp2_data struct would be a better idea, but I was not sure how to handle the alignment issue since there is already the read buffer there:
#define ABP2_MEASUREMENT_RD_SIZE 7
struct abp2_data {
struct device *dev;
const struct abp2_ops *ops;
s32 pmin;
s32 pmax;
[..]
struct {
u32 chan[2];
aligned_s64 timestamp;
} scan;
+ u8 spi_tx_buffer[ABP2_MEASUREMENT_RD_SIZE] __aligned(IIO_DMA_MINALIGN);
u8 buffer[ABP2_MEASUREMENT_RD_SIZE] __aligned(IIO_DMA_MINALIGN);
};
how do I make sure both 7byte buffers are aligned? can I __align twice in a struct as above? or should I align only the first buffer and make it 8bytes long?
I had a close look and even if the SoC's SPI driver supports both DMA and PIO, I've seen it pick PIO every single time while talking to my pressure sensor.
best regards,
peter
--
petre rodan
On Mon, Nov 24, 2025 at 11:08:00PM +0200, Petre Rodan wrote:
> On Mon, Nov 24, 2025 at 08:41:32PM +0200, Andy Shevchenko wrote:
> > On Mon, Nov 24, 2025 at 07:29:06PM +0200, Petre Rodan wrote:
...
> > > > Why explicit assignments? Is it related to some HW register?
> > >
> > > no registers, I just need to ensure the two arrays
> > >
> > > static const char * const abp2_triplet_variants[ABP2_VARIANTS_MAX] = {
> > > [ABP2001BA] = "001BA", [ABP21_6BA] = "1.6BA", [ABP22_5BA] = "2.5BA", ..
> > >
> > > static const struct abp2_range_config abp2_range_config[ABP2_VARIANTS_MAX] = {
> > > [ABP2001BA] = { .pmin = 0, .pmax = 100000 },
> > > [ABP21_6BA] = { .pmin = 0, .pmax = 160000 }, ..
> > >
> > > keep being consistent and are resistant to later editing.
> >
> > So, if it's pure software numbering, just drop assignments in the enum.
>
> so you want the consistency check to be dropped? we have data in two
> different arrays and they must be kept in perfect sync. if I were to remove
> the assignments someone comes a few years in the future, inserts a new device
> in the abp2_triplet_variants array at position 84 out of 113, also inserts a
> new {pmin, pmax} into the abp2_range_config array accidentally at position 83
> and the compiler will be none the wiser.
That's why enum is there. Had I told you to remove it? No! enum should stay
as well as the explicit indexed assignments, assignments in _enum_ should go.
Just look around how other drivers do with enums which are not related to
the HW registers.
> just the day before I had to remove
> a variant because of a typo in the datasheet. I cheat and use a script to
> generate the structs [1], but if I had to modify them by hand, the
> assignments would make sure I delete the proper line.
>
> am I proud of this? no, and I told you my preference. this is just a
> compromise that uses the non-specific match function and still provides a
> guardrail for future editing.
>
> [1] https://codeberg.org/subDIMENSION/lkm_sandbox/src/branch/main/honeywell_abp2030pa/scripts/parse_variants_table.sh
[..]
> > > > So, why can't regmap SPI be used?
> > >
> > > there are no registers, no memory map, just a 'start conversion' and the
> > > equivalent of a 'read conversion' command.
> > > any reason one would use the regmap API in this case?
> >
> > At bare minimum the commit message should have a justification for the choice
> > explaining all this.
>
> I had the justification in the cover letter instead, my bad, will include it in
> the commit message instead.
It's good to have in both.
> > Ideally, try to find a way how to use regmap API. We have several weeks of
> > time for this exercise.
>
> you did not mention why use an API designed for devices with registers and a
> memory map on an IC that has neither.
The regmap provides several facilities that we would like to use in the drivers:
- the generic interface to access to the HW
- the common locking schema that allows to share the same regmap among
different drivers (depends on the functionality of the parts of the HW)
- debugging facilities are available out-of-the-box
> I also have a bughunt in the spi-omap2-mcspi driver related to improper CS
> delays in queued transfers, regmap will probably just be an extra layer of
> abstraction I will have to go thru :/
[..]
> oh, and
>
> struct abp2_spi_buf {
> u8 tx[ABP2_MEASUREMENT_RD_SIZE] __aligned(IIO_DMA_MINALIGN);
> };
>
> static int abp2_spi_init(struct device *dev)
> {
> struct spi_device *spi = to_spi_device(dev);
> struct abp2_spi_buf *buf;
>
> buf = devm_kzalloc(dev, sizeof(*buf), GFP_KERNEL);
>
> > Using devm_*() here is most likely a mistake. What is the object lifetime in
> > comparison to the used device?
>
> I did think that placing this into the abp2_data struct would be a better
> idea, but I was not sure how to handle the alignment issue since there is
> already the read buffer there:
We have drivers in the kernel with two buffers in the same structure.
> #define ABP2_MEASUREMENT_RD_SIZE 7
>
> struct abp2_data {
> struct device *dev;
> const struct abp2_ops *ops;
> s32 pmin;
> s32 pmax;
[..]
> struct {
> u32 chan[2];
> aligned_s64 timestamp;
> } scan;
> + u8 spi_tx_buffer[ABP2_MEASUREMENT_RD_SIZE] __aligned(IIO_DMA_MINALIGN);
> u8 buffer[ABP2_MEASUREMENT_RD_SIZE] __aligned(IIO_DMA_MINALIGN);
> };
>
> how do I make sure both 7byte buffers are aligned? can I __align twice in a
> struct as above? or should I align only the first buffer and make it 8bytes
> long? I had a close look and even if the SoC's SPI driver supports both DMA
> and PIO, I've seen it pick PIO every single time while talking to my pressure
> sensor.
You told you read books about C language...
Alignment is a property of a single member and a data type in general. Each
field of each data type may have it's own (non-default) alignment along with
the object alignment.
...
Homework:
Why do we need both to be aligned? Do you get the idea what is this all about?
--
With Best Regards,
Andy Shevchenko
Hello Andy,
thank you for your feedback thus far.
On Tue, Nov 25, 2025 at 12:54:15AM +0200, Andy Shevchenko wrote:
> > > > > So, why can't regmap SPI be used?
> > > >
> > > > there are no registers, no memory map, just a 'start conversion' and the
> > > > equivalent of a 'read conversion' command.
> > > > any reason one would use the regmap API in this case?
> > >
> > > At bare minimum the commit message should have a justification for the choice
> > > explaining all this.
> >
> > I had the justification in the cover letter instead, my bad, will include it in
> > the commit message instead.
>
> It's good to have in both.
>
> > > Ideally, try to find a way how to use regmap API. We have several weeks of
> > > time for this exercise.
> >
> > you did not mention why use an API designed for devices with registers and a
> > memory map on an IC that has neither.
>
> The regmap provides several facilities that we would like to use in the drivers:
> - the generic interface to access to the HW
in general I agree that having bus functions behind an abstraction layer can lead
to cleaner and less duplicated code in the driver.
however, afaict in this case I still have to use the exact same low level i2c/spi
accesses and hide them behind a regmap_bus instead of an _ops struct.
plus there seems to be duct tape to make things seem to be what they are not.
so the complexity would just go up a notch.
sorry but I feel that this regmap iayer would not be a clean implementation.
I might change my mind in the future of course.
> - the common locking schema that allows to share the same regmap among
> different drivers (depends on the functionality of the parts of the HW)
is it a good idea for anything external expecting a real regmap to interact with
a driver that was made to mascarade using one, lock notwithstanding?
everything about this particular driver is standalone.
> - debugging facilities are available out-of-the-box
in these cases I just use a signal analyzer and compare with the driver's output.
just out of curiosity, you got any pointers on a non-blocking (asynchronous) bus
transfer debug facility?
> We have drivers in the kernel with two buffers in the same structure.
yup. some __align twice just like in my example, some __align just once, some use
a simple buffer placed on the stack with no apparent alignment when sending the
data.
I've been told during an older review that both buffers need to be aligned due to
DMA-related requirements.
as I mentioned before, in my case the SoC's spi driver always uses PIO mode even
if DMA is also implemented. so testing my code always works no matter the alignment.
> > #define ABP2_MEASUREMENT_RD_SIZE 7
> >
> > struct abp2_data {
> > struct device *dev;
> > const struct abp2_ops *ops;
> > s32 pmin;
> > s32 pmax;
> [..]
> > struct {
> > u32 chan[2];
> > aligned_s64 timestamp;
> > } scan;
> > + u8 spi_tx_buffer[ABP2_MEASUREMENT_RD_SIZE] __aligned(IIO_DMA_MINALIGN);
> > u8 buffer[ABP2_MEASUREMENT_RD_SIZE] __aligned(IIO_DMA_MINALIGN);
> > };
> You told you read books about C language...
>
> Alignment is a property of a single member and a data type in general. Each
> field of each data type may have it's own (non-default) alignment along with
> the object alignment.
I have a hard time following this paragraph so I'm forced to use my 'sorry
English ain't one of my first languages' excuse.
I tried the code above, printk-ed the pointers of those arrays (and the timestamp)
and they all get nicely 64byte aligned.
since I've been told in the past both buffers need to be aligned, I guess this
is the code you require?
best regards,
peter
On Thu, Nov 27, 2025 at 09:01:25AM +0200, Petre Rodan wrote:
> On Tue, Nov 25, 2025 at 12:54:15AM +0200, Andy Shevchenko wrote:
...
> > > > > > So, why can't regmap SPI be used?
> > > > >
> > > > > there are no registers, no memory map, just a 'start conversion' and the
> > > > > equivalent of a 'read conversion' command.
> > > > > any reason one would use the regmap API in this case?
> > > >
> > > > At bare minimum the commit message should have a justification for the choice
> > > > explaining all this.
> > >
> > > I had the justification in the cover letter instead, my bad, will include it in
> > > the commit message instead.
> >
> > It's good to have in both.
> >
> > > > Ideally, try to find a way how to use regmap API. We have several weeks of
> > > > time for this exercise.
> > >
> > > you did not mention why use an API designed for devices with registers and a
> > > memory map on an IC that has neither.
> >
> > The regmap provides several facilities that we would like to use in the drivers:
> > - the generic interface to access to the HW
>
> in general I agree that having bus functions behind an abstraction layer can lead
> to cleaner and less duplicated code in the driver.
>
> however, afaict in this case I still have to use the exact same low level i2c/spi
> accesses and hide them behind a regmap_bus instead of an _ops struct.
> plus there seems to be duct tape to make things seem to be what they are not.
> so the complexity would just go up a notch.
>
> sorry but I feel that this regmap iayer would not be a clean implementation.
> I might change my mind in the future of course.
OK.
> > - the common locking schema that allows to share the same regmap among
> > different drivers (depends on the functionality of the parts of the HW)
>
> is it a good idea for anything external expecting a real regmap to interact with
> a driver that was made to mascarade using one, lock notwithstanding?
> everything about this particular driver is standalone.
>
> > - debugging facilities are available out-of-the-box
>
> in these cases I just use a signal analyzer and compare with the driver's output.
> just out of curiosity, you got any pointers on a non-blocking (asynchronous) bus
> transfer debug facility?
I am not sure I got the question right. The regmap has a trace events for read
and write, so it dumps all data goes on the bus. This is done synchronously and
it blocks for a few microseconds to collect the necessary (binary) date in the
trace buffer. I don't think this affects the timings of the real transfers on
the slow busses like SPI or especially I²C.
> > We have drivers in the kernel with two buffers in the same structure.
>
> yup. some __align twice just like in my example, some __align just once, some use
> a simple buffer placed on the stack with no apparent alignment when sending the
> data.
>
> I've been told during an older review that both buffers need to be aligned due to
> DMA-related requirements.
Correct, and this should be done for the buffers that are subject to DMA, PIO is
fine without that. Consideration of Tx/Rx split varies, indeed. And I'm not sure
that the authors of the drivers fully understand when and what alignment is
required. Jonathan once explained to me why only a single alignment is good and
second is not needed, but I forgot that, can't reproduce right now. It's
available on lore archive somewhere.
> as I mentioned before, in my case the SoC's spi driver always uses PIO mode even
> if DMA is also implemented. so testing my code always works no matter the alignment.
[..]
> > > struct {
> > > u32 chan[2];
> > > aligned_s64 timestamp;
> > > } scan;
> > > + u8 spi_tx_buffer[ABP2_MEASUREMENT_RD_SIZE] __aligned(IIO_DMA_MINALIGN);
> > > u8 buffer[ABP2_MEASUREMENT_RD_SIZE] __aligned(IIO_DMA_MINALIGN);
> > > };
> > You told you read books about C language...
> >
> > Alignment is a property of a single member and a data type in general. Each
> > field of each data type may have it's own (non-default) alignment along with
> > the object alignment.
>
> I have a hard time following this paragraph so I'm forced to use my 'sorry
> English ain't one of my first languages' excuse.
OK. I forgot the context of the above, TBH.
--
With Best Regards,
Andy Shevchenko
© 2016 - 2025 Red Hat, Inc.