* [PATCH 0/2] iio: pressure: add Honeywell ABP2 driver
@ 2025-11-22 21:42 Petre Rodan
2025-11-22 21:42 ` [PATCH 1/2] dt-bindings: iio: pressure: add honeywell,abp2030pa Petre Rodan
2025-11-22 21:42 ` [PATCH 2/2] iio: pressure: add Honeywell ABP2 driver Petre Rodan
0 siblings, 2 replies; 13+ messages in thread
From: Petre Rodan @ 2025-11-22 21:42 UTC (permalink / raw)
To: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley
Cc: linux-iio, devicetree, linux-kernel, Petre Rodan
Adds driver for digital Honeywell ABP2 series of board mount
pressure and temperature sensors.
This driver differs quite a bit from the Honeywell ABP series that
is already present in the kernel:
- many more pressure-triplet variations of pressure ranges and
units
- extra end of conversion interrupt feature
- implements both i2c and SPI interfaces
- uses a bidirectional data retrieving protocol (4 wire SPI instead of 3 wire)
- 24+24bit resolution vs 14+11bit on the ABP
Specific low level i2c and spi data transfer API is used instead
of regmap because the protocol is based on simple commands instead
of on a memory map.
Given the fact that the sensor can perform gage and differential
pressure measurements with a full-scale range of down to 500 pascals
the offset and scale are calculated in such a way to provide a result
in pascals, not kilopascals. Just like the Honeywell MPR driver.
For a kilopascal output the scale representation would lose too much
precision.
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>
---
Petre Rodan (2):
dt-bindings: iio: pressure: add honeywell,abp2030pa
iio: pressure: add Honeywell ABP2 driver
.../bindings/iio/pressure/honeywell,abp2030pa.yaml | 133 +++++
MAINTAINERS | 7 +
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 ++++
8 files changed, 970 insertions(+)
---
base-commit: f9e05791642810a0cf6237d39fafd6fec5e0b4bb
change-id: 20251122-honeywell_abp2_driver-9c796ef0c4b2
Best regards,
--
Petre Rodan <petre.rodan@subdimension.ro>
^ permalink raw reply [flat|nested] 13+ messages in thread* [PATCH 1/2] dt-bindings: iio: pressure: add honeywell,abp2030pa 2025-11-22 21:42 [PATCH 0/2] iio: pressure: add Honeywell ABP2 driver Petre Rodan @ 2025-11-22 21:42 ` Petre Rodan 2025-11-23 10:25 ` Krzysztof Kozlowski 2025-11-22 21:42 ` [PATCH 2/2] iio: pressure: add Honeywell ABP2 driver Petre Rodan 1 sibling, 1 reply; 13+ messages in thread From: Petre Rodan @ 2025-11-22 21:42 UTC (permalink / raw) To: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley Cc: linux-iio, devicetree, linux-kernel, Petre Rodan Adds binding for digital Honeywell ABP2 series pressure and temperature sensors. The i2c address is hardcoded and depends on the part number. There is an optional GPIO interrupt that signals the end of conversion. Signed-off-by: Petre Rodan <petre.rodan@subdimension.ro> --- .../bindings/iio/pressure/honeywell,abp2030pa.yaml | 133 +++++++++++++++++++++ MAINTAINERS | 6 + 2 files changed, 139 insertions(+) diff --git a/Documentation/devicetree/bindings/iio/pressure/honeywell,abp2030pa.yaml b/Documentation/devicetree/bindings/iio/pressure/honeywell,abp2030pa.yaml new file mode 100644 index 000000000000..8da39ee43dbc --- /dev/null +++ b/Documentation/devicetree/bindings/iio/pressure/honeywell,abp2030pa.yaml @@ -0,0 +1,133 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/pressure/honeywell,abp2030pa.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Honeywell abp2030pa pressure sensor + +maintainers: + - Petre Rodan <petre.rodan@subdimension.ro> + +description: | + Honeywell pressure sensor of model abp2030pa. + + This sensor has an I2C and SPI interface. + + There are many models with different pressure ranges available. The vendor + calls them "ABP2 series". All of them have an identical programming model and + differ in the pressure range and measurement unit. + + To support different models one needs to specify its pressure triplet. + + For custom silicon chips not covered by the Honeywell ABP2 series datasheet, + the pressure values can be specified manually via honeywell,pmin-pascal and + honeywell,pmax-pascal. + + Specifications about the devices can be found at: + 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 + +properties: + compatible: + const: honeywell,abp2030pa + + reg: + maxItems: 1 + + interrupts: + description: + Optional interrupt for indicating end of conversion. + SPI variants of ABP2 chips do not provide this feature. + maxItems: 1 + + honeywell,pressure-triplet: + description: | + Case-sensitive five character string that defines pressure range, unit + and type as part of the device nomenclature. In the unlikely case of a + custom chip, unset and provide pmin-pascal and pmax-pascal instead. + enum: [001BA, 1.6BA, 2.5BA, 004BA, 006BA, 008BA, 010BA, 012BA, 001BD, + 1.6BD, 2.5BD, 004BD, 001BG, 1.6BG, 2.5BG, 004BG, 006BG, 008BG, + 010BG, 012BG, 001GG, 1.2GG, 100KA, 160KA, 250KA, 001KD, 1.6KD, + 2.5KD, 004KD, 006KD, 010KD, 016KD, 025KD, 040KD, 060KD, 100KD, + 160KD, 250KD, 400KD, 001KG, 1.6KG, 2.5KG, 004KG, 006KG, 010KG, + 016KG, 025KG, 040KG, 060KG, 100KG, 160KG, 250KG, 400KG, 600KG, + 800KG, 250LD, 600LD, 600LG, 2.5MD, 006MD, 010MD, 016MD, 025MD, + 040MD, 060MD, 100MD, 160MD, 250MD, 400MD, 600MD, 006MG, 010MG, + 016MG, 025MG, 040MG, 060MG, 100MG, 160MG, 250MG, 400MG, 600MG, + 001ND, 002ND, 004ND, 005ND, 010ND, 020ND, 030ND, 002NG, 004NG, + 005NG, 010NG, 020NG, 030NG, 015PA, 030PA, 060PA, 100PA, 150PA, + 175PA, 001PD, 005PD, 015PD, 030PD, 060PD, 001PG, 005PG, 015PG, + 030PG, 060PG, 100PG, 150PG, 175PG] + $ref: /schemas/types.yaml#/definitions/string + + honeywell,pmin-pascal: + description: + Minimum pressure value the sensor can measure in pascal. + + honeywell,pmax-pascal: + description: + Maximum pressure value the sensor can measure in pascal. + + spi-max-frequency: + maximum: 800000 + + vdd-supply: + description: provide VDD power to the sensor. + +required: + - compatible + - reg + - vdd-supply + +oneOf: + - required: + - honeywell,pressure-triplet + - required: + - honeywell,pmin-pascal + - honeywell,pmax-pascal + +allOf: + - $ref: /schemas/spi/spi-peripheral-props.yaml + - if: + required: + - honeywell,pressure-triplet + then: + properties: + honeywell,pmin-pascal: false + honeywell,pmax-pascal: false + +additionalProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + #include <dt-bindings/interrupt-controller/irq.h> + i2c { + #address-cells = <1>; + #size-cells = <0>; + + pressure@18 { + compatible = "honeywell,abp2030pa"; + reg = <0x18>; + interrupt-parent = <&gpio3>; + interrupts = <21 IRQ_TYPE_EDGE_RISING>; + + honeywell,pressure-triplet = "001BA"; + vdd-supply = <&vcc_3v3>; + }; + }; + - | + spi { + #address-cells = <1>; + #size-cells = <0>; + + pressure@0 { + compatible = "honeywell,abp2030pa"; + reg = <0>; + spi-max-frequency = <800000>; + + honeywell,pressure-triplet = "001PD"; + vdd-supply = <&vcc_3v3>; + }; + }; +... diff --git a/MAINTAINERS b/MAINTAINERS index 31d98efb1ad1..15b92300acbc 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -11404,6 +11404,12 @@ F: lib/test_hmm* F: mm/hmm* F: tools/testing/selftests/mm/*hmm* +HONEYWELL ABP2030PA PRESSURE SENSOR SERIES IIO DRIVER +M: Petre Rodan <petre.rodan@subdimension.ro> +L: linux-iio@vger.kernel.org +S: Maintained +F: Documentation/devicetree/bindings/iio/pressure/honeywell,abp2030pa.yaml + HONEYWELL HSC030PA PRESSURE SENSOR SERIES IIO DRIVER M: Petre Rodan <petre.rodan@subdimension.ro> L: linux-iio@vger.kernel.org -- 2.51.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 1/2] dt-bindings: iio: pressure: add honeywell,abp2030pa 2025-11-22 21:42 ` [PATCH 1/2] dt-bindings: iio: pressure: add honeywell,abp2030pa Petre Rodan @ 2025-11-23 10:25 ` Krzysztof Kozlowski 0 siblings, 0 replies; 13+ messages in thread From: Krzysztof Kozlowski @ 2025-11-23 10:25 UTC (permalink / raw) To: Petre Rodan Cc: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-iio, devicetree, linux-kernel On Sat, Nov 22, 2025 at 11:42:44PM +0200, Petre Rodan wrote: > Adds binding for digital Honeywell ABP2 series pressure and temperature > sensors. > The i2c address is hardcoded and depends on the part number. > There is an optional GPIO interrupt that signals the end of conversion. s/GPIO//. This is not a GPIO pin in this device, but interrupt pin. How the pin is on the SoC side in your design is irrelevant. No need to resend just for that. Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org> Best regards, Krzysztof ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/2] iio: pressure: add Honeywell ABP2 driver 2025-11-22 21:42 [PATCH 0/2] iio: pressure: add Honeywell ABP2 driver Petre Rodan 2025-11-22 21:42 ` [PATCH 1/2] dt-bindings: iio: pressure: add honeywell,abp2030pa Petre Rodan @ 2025-11-22 21:42 ` Petre Rodan 2025-11-24 11:48 ` Andy Shevchenko 1 sibling, 1 reply; 13+ messages in thread From: Petre Rodan @ 2025-11-22 21:42 UTC (permalink / raw) To: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley Cc: linux-iio, devicetree, linux-kernel, Petre Rodan 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.0 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] iio: pressure: add Honeywell ABP2 driver 2025-11-22 21:42 ` [PATCH 2/2] iio: pressure: add Honeywell ABP2 driver Petre Rodan @ 2025-11-24 11:48 ` Andy Shevchenko 2025-11-24 17:29 ` Petre Rodan 2025-12-06 20:13 ` Jonathan Cameron 0 siblings, 2 replies; 13+ messages in thread From: Andy Shevchenko @ 2025-11-24 11:48 UTC (permalink / raw) To: Petre Rodan Cc: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-iio, devicetree, linux-kernel On 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] iio: pressure: add Honeywell ABP2 driver 2025-11-24 11:48 ` Andy Shevchenko @ 2025-11-24 17:29 ` Petre Rodan 2025-11-24 18:41 ` Andy Shevchenko 2025-12-06 20:13 ` Jonathan Cameron 1 sibling, 1 reply; 13+ messages in thread From: Petre Rodan @ 2025-11-24 17:29 UTC (permalink / raw) To: Andy Shevchenko Cc: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-iio, devicetree, linux-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] iio: pressure: add Honeywell ABP2 driver 2025-11-24 17:29 ` Petre Rodan @ 2025-11-24 18:41 ` Andy Shevchenko 2025-11-24 21:08 ` Petre Rodan 0 siblings, 1 reply; 13+ messages in thread From: Andy Shevchenko @ 2025-11-24 18:41 UTC (permalink / raw) To: Petre Rodan Cc: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-iio, devicetree, linux-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] iio: pressure: add Honeywell ABP2 driver 2025-11-24 18:41 ` Andy Shevchenko @ 2025-11-24 21:08 ` Petre Rodan 2025-11-24 22:54 ` Andy Shevchenko 0 siblings, 1 reply; 13+ messages in thread From: Petre Rodan @ 2025-11-24 21:08 UTC (permalink / raw) To: Andy Shevchenko Cc: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-iio, devicetree, linux-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] iio: pressure: add Honeywell ABP2 driver 2025-11-24 21:08 ` Petre Rodan @ 2025-11-24 22:54 ` Andy Shevchenko 2025-11-27 7:01 ` Petre Rodan 0 siblings, 1 reply; 13+ messages in thread From: Andy Shevchenko @ 2025-11-24 22:54 UTC (permalink / raw) To: Petre Rodan Cc: Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-iio, devicetree, linux-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] iio: pressure: add Honeywell ABP2 driver 2025-11-24 22:54 ` Andy Shevchenko @ 2025-11-27 7:01 ` Petre Rodan 2025-11-27 8:37 ` Andy Shevchenko 0 siblings, 1 reply; 13+ messages in thread From: Petre Rodan @ 2025-11-27 7:01 UTC (permalink / raw) To: Andy Shevchenko Cc: Jonathan Cameron, David Lechner, Nuno S?, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-iio, devicetree, linux-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] iio: pressure: add Honeywell ABP2 driver 2025-11-27 7:01 ` Petre Rodan @ 2025-11-27 8:37 ` Andy Shevchenko 2025-12-06 20:29 ` Jonathan Cameron 0 siblings, 1 reply; 13+ messages in thread From: Andy Shevchenko @ 2025-11-27 8:37 UTC (permalink / raw) To: Petre Rodan Cc: Jonathan Cameron, David Lechner, Nuno S?, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-iio, devicetree, linux-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] iio: pressure: add Honeywell ABP2 driver 2025-11-27 8:37 ` Andy Shevchenko @ 2025-12-06 20:29 ` Jonathan Cameron 0 siblings, 0 replies; 13+ messages in thread From: Jonathan Cameron @ 2025-12-06 20:29 UTC (permalink / raw) To: Andy Shevchenko Cc: Petre Rodan, David Lechner, Nuno S?, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-iio, devicetree, linux-kernel > > > 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. The logic behind this __aligned(IIO_DMA_MINALIGN) is all about who can mess with stuff in a given cacheline whilst DMA is ongoing. There are DMA masters that, when only actually using a small part of a cacheline will copy the whole thing to local storage, run the DMA and update the bits they are using before copying the whole cacheline back again. Note there are other DMA master designs that don't do this - so you may never see it! IIRC I only hit this once many years ago - it is very very hard to debug though! So if anyone else (e.g. the CPU running something parallel) touches other data in the cacheline (64bytes or so) it will be overwritten with the stale values. The iio_priv() structure embedded in the larger allocation has forced alignment to allow us then to use an embedded __aligned(IIO_DMA_MINALIGN) to force padding between other elements of iio_priv() structure and the start of our new buffer. We have to be careful to not put anything that might be concurrently from a different element (e.g. CPU) after these. E.g. struct bob { int a; u8 rx[4] __aligned(IIO_DMA_MINALIGN); int vitaldata; } could result in stale values in vitaldata. The assumption behind being able to do struct bob { int a; u8 rx[4] __aligned(IIO_DMA_MINALIGN); u8 tx[4]; //no marking } is that both rx and tx are handed to the DMA engine for an operation (some lock or similar protects them anyway) and DMA engine that managed to corrupt data in the copy of the cacheline that was entirely in its control is horribly broken anyway and it's up to the SPI layer or similar to bounce buffers if that's the case. Short of randomly writing stuff it never should touch in it's own local copy of the cacheline I have no idea how this would happen. Jonathan ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/2] iio: pressure: add Honeywell ABP2 driver 2025-11-24 11:48 ` Andy Shevchenko 2025-11-24 17:29 ` Petre Rodan @ 2025-12-06 20:13 ` Jonathan Cameron 1 sibling, 0 replies; 13+ messages in thread From: Jonathan Cameron @ 2025-12-06 20:13 UTC (permalink / raw) To: Andy Shevchenko Cc: Petre Rodan, David Lechner, Nuno Sá, Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley, linux-iio, devicetree, linux-kernel > ... > > > +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? Not currently. We should revisit if we this ever actually proved useful to set. It dates back to very early in IIO where I assumed we would want to differential devices that only do buffered capture from those that can do that and single shot. I'm not sure we actually have any in that category though :( So maybe we can justify dropping this across all drivers at somepoint or as you say just set it as a default value. Jonathan ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2025-12-06 20:30 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-11-22 21:42 [PATCH 0/2] iio: pressure: add Honeywell ABP2 driver Petre Rodan 2025-11-22 21:42 ` [PATCH 1/2] dt-bindings: iio: pressure: add honeywell,abp2030pa Petre Rodan 2025-11-23 10:25 ` Krzysztof Kozlowski 2025-11-22 21:42 ` [PATCH 2/2] iio: pressure: add Honeywell ABP2 driver Petre Rodan 2025-11-24 11:48 ` Andy Shevchenko 2025-11-24 17:29 ` Petre Rodan 2025-11-24 18:41 ` Andy Shevchenko 2025-11-24 21:08 ` Petre Rodan 2025-11-24 22:54 ` Andy Shevchenko 2025-11-27 7:01 ` Petre Rodan 2025-11-27 8:37 ` Andy Shevchenko 2025-12-06 20:29 ` Jonathan Cameron 2025-12-06 20:13 ` Jonathan Cameron
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).