[PATCH v2 0/2] iio: pressure: add driver and bindings for adp810

[PATCH v2 0/2] iio: pressure: add driver and bindings for adp810

[Akhilesh Patil]

This patch series adds support for aosong adp810 differential pressure and
temperature sensor driver in the IIO subsystem.

Patch 1: Adds bindings for this hardware.
Patch 2: Adds driver code with device tree support.

Overview of adp810:
This is digital differential pressure and temperature sensor from aosong under
the brand name of ASAIR. This sensor can measure pressure from -500 to +500Pa
and temperature from -40 to +85 degree. It provides simple protocol to measure
readings over I2C bus interface.

How to read from sensor (Protocol)?
To read from sensor, i2c master needs to send measure command 0x372d to
start the data acquisition. Then host/master should wait for minimum 10ms for data
to be ready before reading. Post this delay i2c master can read 9 bytes of
measurement data which includes - pressure(u16): crc(u8): temperature(u16): crc(u8)
scale factor (u16): crc(8).
Host/master can optionally verify crc for data integrity. Read sequence can be
terminated anytime by sending NAK.

Datasheet: https://aosong.com/userfiles/files/media/Datasheet%20ADP810-Digital.pdf

Testing:
Driver is tested on Texas Instruments am62x sk board by connecting sensor at i2c-2.
Data communication is validated with i2c bus at 100KHz and 400KHz using logic analyzer.
Sensor values are read using iio subsystem's sysfs interface.

Changes in v2:
- Wrapped yaml binding description to 80 lines.
- Dropped block scalar ' | ' from binding description.
- Carry forward Reviewed-by tag from Krzysztof on device tree binding.
- Grammar and spelling fixes at multiple places.
- Ordered makefile alphabetically.
- Ordered include files alphabetically and used IWYU principle
- Explicitly mentioned unit of measure latency macro in MS (milliseconds)
- Added inline comments for explaining CRC8 polynomial for CRC calculation 
- Used scoped_guard() for mutex for safe and clean lock handling.
- Used resource managed mutex_init() -> devm_mutex_init()
- Removed dead code in _probe() function.
- Used __be16 and related helpers to handle big endian data processing.
- Apply reverse xmas tree guideline while declaring local variables if possible.
- Used parent device pointer in dev_err() calls.
- Hardcode device name string in _probe() function for simplicity.
- Made default return value of _probe() function to 0.
- Rebased and retested driver on top of 6.18-rc1
- Link to v1: https://lore.kernel.org/lkml/cover.1760184859.git.akhilesh@ee.iitb.ac.in/

Looking forward for feedback and suggestions.

Regards,
Akhilesh

Akhilesh Patil (2):
  dt-bindings: iio: pressure: Add Aosong adp810
  iio: pressure: adp810: Add driver for adp810 sensor

 .../bindings/iio/pressure/aosong,adp810.yaml  |  46 ++++
 MAINTAINERS                                   |   7 +
 drivers/iio/pressure/Kconfig                  |  12 +
 drivers/iio/pressure/Makefile                 |   8 +-
 drivers/iio/pressure/adp810.c                 | 212 ++++++++++++++++++
 5 files changed, 281 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/pressure/aosong,adp810.yaml
 create mode 100644 drivers/iio/pressure/adp810.c

-- 
2.34.1

[PATCH v2 1/2] dt-bindings: iio: pressure: Add Aosong adp810

[Akhilesh Patil]

Add bindings for adp810 differential pressure and temperature
sensor. This sensor communicates over I2C with CRC support and
can measure pressure in the range -500 to 500Pa and temperature
in the range -40 to +85 degree celsius.

Signed-off-by: Akhilesh Patil <akhilesh@ee.iitb.ac.in>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
 .../bindings/iio/pressure/aosong,adp810.yaml  | 46 +++++++++++++++++++
 1 file changed, 46 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/pressure/aosong,adp810.yaml

diff --git a/Documentation/devicetree/bindings/iio/pressure/aosong,adp810.yaml b/Documentation/devicetree/bindings/iio/pressure/aosong,adp810.yaml
new file mode 100644
index 000000000000..b35eb63531c2
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/pressure/aosong,adp810.yaml
@@ -0,0 +1,46 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/pressure/aosong,adp810.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: aosong adp810 differential pressure sensor
+
+maintainers:
+  - Akhilesh Patil <akhilesh@ee.iitb.ac.in>
+
+description:
+  ADP810 is differential pressure and temperature sensor. It has I2C bus
+  interface with fixed address of 0x25. This sensor supports 8 bit CRC for
+  reliable data transfer. It can measure differential pressure in the
+  range -500 to 500Pa and temperate in the range -40 to +85 degree celsius.
+
+properties:
+  compatible:
+    enum:
+      - aosong,adp810
+
+  reg:
+    maxItems: 1
+
+  vdd-supply: true
+
+required:
+  - compatible
+  - reg
+  - vdd-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        pressure-sensor@25 {
+            compatible = "aosong,adp810";
+            reg = <0x25>;
+            vdd-supply = <&vdd_regulator>;
+        };
+    };
+
-- 
2.34.1

[PATCH v2 2/2] iio: pressure: adp810: Add driver for adp810 sensor

[Akhilesh Patil]

Add driver for Aosong adp810 differential pressure and temperature sensor.
This sensor provides an I2C interface for reading data.
Calculate CRC of the data received using standard crc8 library to verify
data integrity.

Tested on TI am62x sk board with sensor connected at i2c-2.

Signed-off-by: Akhilesh Patil <akhilesh@ee.iitb.ac.in>
---
 MAINTAINERS                   |   7 ++
 drivers/iio/pressure/Kconfig  |  12 ++
 drivers/iio/pressure/Makefile |   8 +-
 drivers/iio/pressure/adp810.c | 212 ++++++++++++++++++++++++++++++++++
 4 files changed, 235 insertions(+), 4 deletions(-)
 create mode 100644 drivers/iio/pressure/adp810.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 46126ce2f968..bb30f7b31cb7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3745,6 +3745,13 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/iio/chemical/aosong,ags02ma.yaml
 F:	drivers/iio/chemical/ags02ma.c
 
+AOSONG ADP810 DIFFERENTIAL PRESSURE SENSOR DRIVER
+M:	Akhilesh Patil <akhilesh@ee.iitb.ac.in>
+L:	linux-iio@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/iio/pressure/aosong,adp810.yaml
+F:	drivers/iio/pressure/adp810.c
+
 ASC7621 HARDWARE MONITOR DRIVER
 M:	George Joseph <george.joseph@fairview5.com>
 L:	linux-hwmon@vger.kernel.org
diff --git a/drivers/iio/pressure/Kconfig b/drivers/iio/pressure/Kconfig
index d2cb8c871f6a..2fe9dc90cceb 100644
--- a/drivers/iio/pressure/Kconfig
+++ b/drivers/iio/pressure/Kconfig
@@ -339,4 +339,16 @@ config ZPA2326_SPI
 	tristate
 	select REGMAP_SPI
 
+config ADP810
+	tristate "Aosong adp810 differential pressure and temperature sensor"
+	depends on I2C
+	select CRC8
+	help
+	  Say yes here to build adp810 differential pressure and temperature
+	  sensor driver. ADP810 can measure pressure range up to 500Pa.
+	  It supports an I2C interface for data communication.
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called adp810
+
 endmenu
diff --git a/drivers/iio/pressure/Makefile b/drivers/iio/pressure/Makefile
index 6482288e07ee..a21443e992b9 100644
--- a/drivers/iio/pressure/Makefile
+++ b/drivers/iio/pressure/Makefile
@@ -5,6 +5,7 @@
 
 # When adding new entries keep the list in alphabetical order
 obj-$(CONFIG_ABP060MG) += abp060mg.o
+obj-$(CONFIG_ADP810) += adp810.o
 obj-$(CONFIG_ROHM_BM1390) += rohm-bm1390.o
 obj-$(CONFIG_BMP280) += bmp280.o
 bmp280-objs := bmp280-core.o bmp280-regmap.o
@@ -15,6 +16,7 @@ obj-$(CONFIG_DPS310) += dps310.o
 obj-$(CONFIG_IIO_CROS_EC_BARO) += cros_ec_baro.o
 obj-$(CONFIG_HID_SENSOR_PRESS)   += hid-sensor-press.o
 obj-$(CONFIG_HP03) += hp03.o
+obj-$(CONFIG_HP206C) += hp206c.o
 obj-$(CONFIG_HSC030PA) += hsc030pa.o
 obj-$(CONFIG_HSC030PA_I2C) += hsc030pa_i2c.o
 obj-$(CONFIG_HSC030PA_SPI) += hsc030pa_spi.o
@@ -34,11 +36,9 @@ obj-$(CONFIG_SDP500) += sdp500.o
 obj-$(CONFIG_IIO_ST_PRESS) += st_pressure.o
 st_pressure-y := st_pressure_core.o
 st_pressure-$(CONFIG_IIO_BUFFER) += st_pressure_buffer.o
+obj-$(CONFIG_IIO_ST_PRESS_I2C) += st_pressure_i2c.o
+obj-$(CONFIG_IIO_ST_PRESS_SPI) += st_pressure_spi.o
 obj-$(CONFIG_T5403) += t5403.o
-obj-$(CONFIG_HP206C) += hp206c.o
 obj-$(CONFIG_ZPA2326) += zpa2326.o
 obj-$(CONFIG_ZPA2326_I2C) += zpa2326_i2c.o
 obj-$(CONFIG_ZPA2326_SPI) += zpa2326_spi.o
-
-obj-$(CONFIG_IIO_ST_PRESS_I2C) += st_pressure_i2c.o
-obj-$(CONFIG_IIO_ST_PRESS_SPI) += st_pressure_spi.o
diff --git a/drivers/iio/pressure/adp810.c b/drivers/iio/pressure/adp810.c
new file mode 100644
index 000000000000..c2f3b5f7a1f9
--- /dev/null
+++ b/drivers/iio/pressure/adp810.c
@@ -0,0 +1,212 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2025 Akhilesh Patil <akhilesh@ee.iitb.ac.in>
+ *
+ * Driver for adp810 pressure and temperature sensor
+ * Datasheet:
+ *   https://aosong.com/userfiles/files/media/Datasheet%20ADP810-Digital.pdf
+ */
+
+#include <linux/crc8.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/unaligned.h>
+
+#include <linux/iio/iio.h>
+
+/*
+ * Time taken in ms by sensor to do measurements after triggering.
+ * As per datasheet 10ms is sufficient but we define 30ms for better margin.
+ */
+#define ADP810_MEASURE_LATENCY_MS	30
+/* Trigger command to send to start measurement by the sensor */
+#define ADP810_TRIGGER_COMMAND		0x2d37
+/*
+ * Refer section 5.4 checksum calculation from datasheet.
+ * This sensor uses CRC polynomial x^8 + x^5 + x^4 + 1 (0x31)
+ */
+#define ADP810_CRC8_POLYNOMIAL		0x31
+
+DECLARE_CRC8_TABLE(crc_table);
+
+struct adp810_read_buf {
+	__be16 dp;
+	u8 dp_crc;
+	__be16 tmp;
+	u8 tmp_crc;
+	__be16 sf;
+	u8 sf_crc;
+} __packed;
+
+struct adp810_data {
+	struct i2c_client *client;
+	/* Use lock to synchronize access to device during read sequence */
+	struct mutex lock;
+};
+
+static int adp810_measure(struct adp810_data *data, struct adp810_read_buf *buf)
+{
+	struct i2c_client *client = data->client;
+	struct device *dev = &client->dev;
+	int ret;
+	u16 trig_cmd = ADP810_TRIGGER_COMMAND;
+
+	/* Send trigger to the sensor for measurement */
+	ret = i2c_master_send(client, (char *)&trig_cmd, sizeof(u16));
+	if (ret < 0) {
+		dev_err(dev, "Error sending trigger command\n");
+		return ret;
+	}
+
+	/*
+	 * Wait for the sensor to acquire data. As per datasheet section 5.3.1,
+	 * wait for at least 10ms before reading measurements from the sensor.
+	 */
+	msleep(ADP810_MEASURE_LATENCY_MS);
+
+	/* Read sensor values */
+	ret = i2c_master_recv(client, (char *)buf, sizeof(*buf));
+	if (ret < 0) {
+		dev_err(dev, "Error reading from sensor\n");
+		return ret;
+	}
+
+	/* CRC checks */
+	crc8_populate_msb(crc_table, ADP810_CRC8_POLYNOMIAL);
+	if (buf->dp_crc != crc8(crc_table, (u8 *)&buf->dp, 0x2, CRC8_INIT_VALUE)) {
+		dev_err(dev, "CRC error for pressure\n");
+		return -EIO;
+	}
+
+	if (buf->tmp_crc != crc8(crc_table, (u8 *)&buf->tmp, 0x2, CRC8_INIT_VALUE)) {
+		dev_err(dev, "CRC error for temperature\n");
+		return -EIO;
+	}
+
+	if (buf->sf_crc != crc8(crc_table, (u8 *)&buf->sf, 0x2, CRC8_INIT_VALUE)) {
+		dev_err(dev, "CRC error for scale\n");
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int adp810_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan,
+			   int *val, int *val2, long mask)
+{
+	struct adp810_data *data = iio_priv(indio_dev);
+	struct device *dev = &data->client->dev;
+	struct adp810_read_buf buf = {0};
+	int ret;
+
+	scoped_guard(mutex, &data->lock) {
+		ret = adp810_measure(data, &buf);
+		if (ret) {
+			dev_err(dev, "Failed to read from device\n");
+			return ret;
+		}
+	}
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		switch (chan->type) {
+		case IIO_PRESSURE:
+			*val = get_unaligned_be16(&buf.dp);
+			return IIO_VAL_INT;
+		case IIO_TEMP:
+			*val = get_unaligned_be16(&buf.tmp);
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->type) {
+		case IIO_PRESSURE:
+			*val = get_unaligned_be16(&buf.sf);
+			return IIO_VAL_INT;
+		case IIO_TEMP:
+			*val = 200;
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info adp810_info = {
+	.read_raw	= adp810_read_raw,
+};
+
+static const struct iio_chan_spec adp810_channels[] = {
+	{
+		.type = IIO_PRESSURE,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
+	},
+	{
+		.type = IIO_TEMP,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
+	},
+};
+
+static int adp810_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct iio_dev *indio_dev;
+	struct adp810_data *data;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	data = iio_priv(indio_dev);
+	data->client = client;
+
+	ret = devm_mutex_init(dev, &data->lock);
+	if (ret)
+		return ret;
+
+	indio_dev->name = "adp810";
+	indio_dev->channels = adp810_channels;
+	indio_dev->num_channels = ARRAY_SIZE(adp810_channels);
+	indio_dev->info = &adp810_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	ret = devm_iio_device_register(dev, indio_dev);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to register IIO device\n");
+
+	return 0;
+}
+
+static const struct i2c_device_id adp810_id_table[] = {
+	{ "adp810" },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, adp810_id_table);
+
+static const struct of_device_id adp810_of_table[] = {
+	{ .compatible = "aosong,adp810" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, adp810_of_table);
+
+static struct i2c_driver adp810_driver = {
+	.driver = {
+		.name = "adp810",
+		.of_match_table = adp810_of_table,
+	},
+	.probe	= adp810_probe,
+	.id_table = adp810_id_table,
+};
+module_i2c_driver(adp810_driver);
+
+MODULE_AUTHOR("Akhilesh Patil <akhilesh@ee.iitb.ac.in>");
+MODULE_DESCRIPTION("Driver for Aosong ADP810 sensor");
+MODULE_LICENSE("GPL");
-- 
2.34.1

Re: [PATCH v2 2/2] iio: pressure: adp810: Add driver for adp810 sensor

[Jonathan Cameron]

On Mon, 13 Oct 2025 22:32:35 +0530
Akhilesh Patil <akhilesh@ee.iitb.ac.in> wrote:

> Add driver for Aosong adp810 differential pressure and temperature sensor.
> This sensor provides an I2C interface for reading data.
> Calculate CRC of the data received using standard crc8 library to verify
> data integrity.
> 
> Tested on TI am62x sk board with sensor connected at i2c-2.
> 
> Signed-off-by: Akhilesh Patil <akhilesh@ee.iitb.ac.in>

A few comments inline and it seems your rebase when wrong and you've
picked up unrelated build file changes.

Thanks

Jonathan

> diff --git a/drivers/iio/pressure/Makefile b/drivers/iio/pressure/Makefile
> index 6482288e07ee..a21443e992b9 100644
> --- a/drivers/iio/pressure/Makefile
> +++ b/drivers/iio/pressure/Makefile
> @@ -5,6 +5,7 @@
>  
>  # When adding new entries keep the list in alphabetical order
>  obj-$(CONFIG_ABP060MG) += abp060mg.o
> +obj-$(CONFIG_ADP810) += adp810.o
>  obj-$(CONFIG_ROHM_BM1390) += rohm-bm1390.o
>  obj-$(CONFIG_BMP280) += bmp280.o
>  bmp280-objs := bmp280-core.o bmp280-regmap.o
> @@ -15,6 +16,7 @@ obj-$(CONFIG_DPS310) += dps310.o
>  obj-$(CONFIG_IIO_CROS_EC_BARO) += cros_ec_baro.o
>  obj-$(CONFIG_HID_SENSOR_PRESS)   += hid-sensor-press.o
>  obj-$(CONFIG_HP03) += hp03.o
> +obj-$(CONFIG_HP206C) += hp206c.o
>  obj-$(CONFIG_HSC030PA) += hsc030pa.o
>  obj-$(CONFIG_HSC030PA_I2C) += hsc030pa_i2c.o
>  obj-$(CONFIG_HSC030PA_SPI) += hsc030pa_spi.o
> @@ -34,11 +36,9 @@ obj-$(CONFIG_SDP500) += sdp500.o
>  obj-$(CONFIG_IIO_ST_PRESS) += st_pressure.o
>  st_pressure-y := st_pressure_core.o
>  st_pressure-$(CONFIG_IIO_BUFFER) += st_pressure_buffer.o
> +obj-$(CONFIG_IIO_ST_PRESS_I2C) += st_pressure_i2c.o
> +obj-$(CONFIG_IIO_ST_PRESS_SPI) += st_pressure_spi.o
>  obj-$(CONFIG_T5403) += t5403.o
> -obj-$(CONFIG_HP206C) += hp206c.o

Rebase gone wrong I assume.  

>  obj-$(CONFIG_ZPA2326) += zpa2326.o
>  obj-$(CONFIG_ZPA2326_I2C) += zpa2326_i2c.o
>  obj-$(CONFIG_ZPA2326_SPI) += zpa2326_spi.o
> -
> -obj-$(CONFIG_IIO_ST_PRESS_I2C) += st_pressure_i2c.o
> -obj-$(CONFIG_IIO_ST_PRESS_SPI) += st_pressure_spi.o
> diff --git a/drivers/iio/pressure/adp810.c b/drivers/iio/pressure/adp810.c
> new file mode 100644
> index 000000000000..c2f3b5f7a1f9
> --- /dev/null
> +++ b/drivers/iio/pressure/adp810.c
> @@ -0,0 +1,212 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2025 Akhilesh Patil <akhilesh@ee.iitb.ac.in>
> + *
> + * Driver for adp810 pressure and temperature sensor
> + * Datasheet:
> + *   https://aosong.com/userfiles/files/media/Datasheet%20ADP810-Digital.pdf
> + */
> +
> +#include <linux/crc8.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/unaligned.h>
This is a very small set of includes.  Please follow include what you use (IWYU)
principles (loosely - there are a few headers that never make sense to include
directly).  So I'd definitely expect things like mutex.h here.
dev_printk.h etc.

> +
> +#include <linux/iio/iio.h>

> +
> +static int adp810_measure(struct adp810_data *data, struct adp810_read_buf *buf)
> +{
> +	struct i2c_client *client = data->client;
> +	struct device *dev = &client->dev;
> +	int ret;
> +	u16 trig_cmd = ADP810_TRIGGER_COMMAND;
> +
> +	/* Send trigger to the sensor for measurement */
> +	ret = i2c_master_send(client, (char *)&trig_cmd, sizeof(u16));

sizeof(trig_cmd)

I think it is vanishingly unlikely to matter but in theory i2c_master_send()
could return 1 and only one byte made it to device.
So it's common to have 
	if (ret < 0)...
		....
	if (ret != sizeof(trig_cmd))
		....

> +	if (ret < 0) {
> +		dev_err(dev, "Error sending trigger command\n");
> +		return ret;
> +	}
> +
> +	/*
> +	 * Wait for the sensor to acquire data. As per datasheet section 5.3.1,
> +	 * wait for at least 10ms before reading measurements from the sensor.
> +	 */
> +	msleep(ADP810_MEASURE_LATENCY_MS);
> +
> +	/* Read sensor values */
> +	ret = i2c_master_recv(client, (char *)buf, sizeof(*buf));
> +	if (ret < 0) {

Same potential issue for short reads as for the write above.

I don't recall seeing anything to say we either got full length or
error code but maybe that changed at somepoint to make this interface easier to use.


> +		dev_err(dev, "Error reading from sensor\n");
> +		return ret;
> +	}
> +
> +	/* CRC checks */
> +	crc8_populate_msb(crc_table, ADP810_CRC8_POLYNOMIAL);
> +	if (buf->dp_crc != crc8(crc_table, (u8 *)&buf->dp, 0x2, CRC8_INIT_VALUE)) {
> +		dev_err(dev, "CRC error for pressure\n");
> +		return -EIO;
> +	}
> +
> +	if (buf->tmp_crc != crc8(crc_table, (u8 *)&buf->tmp, 0x2, CRC8_INIT_VALUE)) {
> +		dev_err(dev, "CRC error for temperature\n");
> +		return -EIO;
> +	}
> +
> +	if (buf->sf_crc != crc8(crc_table, (u8 *)&buf->sf, 0x2, CRC8_INIT_VALUE)) {
> +		dev_err(dev, "CRC error for scale\n");
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static int adp810_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val, int *val2, long mask)
> +{
> +	struct adp810_data *data = iio_priv(indio_dev);
> +	struct device *dev = &data->client->dev;
> +	struct adp810_read_buf buf = {0};

Not a big thing but slight preference for { }.
It's a messy thing wrt to the c spec which never talked about holes
and this construct until recently.  However the kernel has build tests
that verify that all compilers will zero the holes with the { }
syntax and that is what the C standards folk have put in the spec now
as meaning whole structure including holes.

> +	int ret;
> +
> +	scoped_guard(mutex, &data->lock) {
> +		ret = adp810_measure(data, &buf);
> +		if (ret) {
> +			dev_err(dev, "Failed to read from device\n");
> +			return ret;
> +		}
> +	}
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		switch (chan->type) {
> +		case IIO_PRESSURE:
> +			*val = get_unaligned_be16(&buf.dp);
> +			return IIO_VAL_INT;
> +		case IIO_TEMP:
> +			*val = get_unaligned_be16(&buf.tmp);
> +			return IIO_VAL_INT;
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->type) {
> +		case IIO_PRESSURE:
> +			*val = get_unaligned_be16(&buf.sf);
> +			return IIO_VAL_INT;
> +		case IIO_TEMP:
> +			*val = 200;
> +			return IIO_VAL_INT;
> +		default:
> +			return -EINVAL;
> +		}
> +	default:
> +		return -EINVAL;
> +	}
> +}

Re: [PATCH v2 2/2] iio: pressure: adp810: Add driver for adp810 sensor

[Akhilesh Patil]

On Sat, Oct 18, 2025 at 05:47:46PM +0100, Jonathan Cameron wrote:
> On Mon, 13 Oct 2025 22:32:35 +0530
> Akhilesh Patil <akhilesh@ee.iitb.ac.in> wrote:
> 
> > Add driver for Aosong adp810 differential pressure and temperature sensor.
> > This sensor provides an I2C interface for reading data.
> > Calculate CRC of the data received using standard crc8 library to verify
> > data integrity.
> > 
> > Tested on TI am62x sk board with sensor connected at i2c-2.
> > 
> > Signed-off-by: Akhilesh Patil <akhilesh@ee.iitb.ac.in>
> 
> A few comments inline and it seems your rebase when wrong and you've
> picked up unrelated build file changes.
> 
> Thanks
> 
> Jonathan

Hi Jonathan, Thanks for the review, I will share v3 addressing these comments.

Regards,
Akhilesh

> 
> > diff --git a/drivers/iio/pressure/Makefile b/drivers/iio/pressure/Makefile
> > index 6482288e07ee..a21443e992b9 100644
> > --- a/drivers/iio/pressure/Makefile
> > +++ b/drivers/iio/pressure/Makefile
> > @@ -5,6 +5,7 @@
> >  
> >  # When adding new entries keep the list in alphabetical order
> >  obj-$(CONFIG_ABP060MG) += abp060mg.o
> > +obj-$(CONFIG_ADP810) += adp810.o
> >  obj-$(CONFIG_ROHM_BM1390) += rohm-bm1390.o
> >  obj-$(CONFIG_BMP280) += bmp280.o
> >  bmp280-objs := bmp280-core.o bmp280-regmap.o
> > @@ -15,6 +16,7 @@ obj-$(CONFIG_DPS310) += dps310.o
> >  obj-$(CONFIG_IIO_CROS_EC_BARO) += cros_ec_baro.o
> >  obj-$(CONFIG_HID_SENSOR_PRESS)   += hid-sensor-press.o
> >  obj-$(CONFIG_HP03) += hp03.o
> > +obj-$(CONFIG_HP206C) += hp206c.o
> >  obj-$(CONFIG_HSC030PA) += hsc030pa.o
> >  obj-$(CONFIG_HSC030PA_I2C) += hsc030pa_i2c.o
> >  obj-$(CONFIG_HSC030PA_SPI) += hsc030pa_spi.o
> > @@ -34,11 +36,9 @@ obj-$(CONFIG_SDP500) += sdp500.o
> >  obj-$(CONFIG_IIO_ST_PRESS) += st_pressure.o
> >  st_pressure-y := st_pressure_core.o
> >  st_pressure-$(CONFIG_IIO_BUFFER) += st_pressure_buffer.o
> > +obj-$(CONFIG_IIO_ST_PRESS_I2C) += st_pressure_i2c.o
> > +obj-$(CONFIG_IIO_ST_PRESS_SPI) += st_pressure_spi.o
> >  obj-$(CONFIG_T5403) += t5403.o
> > -obj-$(CONFIG_HP206C) += hp206c.o
> 
> Rebase gone wrong I assume.  

These are intentional changes.

This addresses Andy's suggestion in v1, to keep entries alphabetically
arranged in Makefile. Along with adp810 location, fixed other files as well
hp206 and st_pressure_* to make entries alphabetically arranged in
the entire Makefile.

> 
> >  obj-$(CONFIG_ZPA2326) += zpa2326.o
> >  obj-$(CONFIG_ZPA2326_I2C) += zpa2326_i2c.o
> >  obj-$(CONFIG_ZPA2326_SPI) += zpa2326_spi.o
> > -
> > -obj-$(CONFIG_IIO_ST_PRESS_I2C) += st_pressure_i2c.o
> > -obj-$(CONFIG_IIO_ST_PRESS_SPI) += st_pressure_spi.o
> > diff --git a/drivers/iio/pressure/adp810.c b/drivers/iio/pressure/adp810.c
> > new file mode 100644
> > index 000000000000..c2f3b5f7a1f9
> > --- /dev/null
> > +++ b/drivers/iio/pressure/adp810.c
> > @@ -0,0 +1,212 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright (c) 2025 Akhilesh Patil <akhilesh@ee.iitb.ac.in>
> > + *
> > + * Driver for adp810 pressure and temperature sensor
> > + * Datasheet:
> > + *   https://aosong.com/userfiles/files/media/Datasheet%20ADP810-Digital.pdf
> > + */
> > +
> > +#include <linux/crc8.h>
> > +#include <linux/delay.h>
> > +#include <linux/i2c.h>
> > +#include <linux/module.h>
> > +#include <linux/unaligned.h>
> This is a very small set of includes.  Please follow include what you use (IWYU)
> principles (loosely - there are a few headers that never make sense to include
> directly).  So I'd definitely expect things like mutex.h here.
> dev_printk.h etc.

ACK.
Added mutex.h, dev_printk.h along with other includes - cleanup.h,
device.h, iio/types.h, mod_devicetable.h to follow IWYU.

> 
> > +
> > +#include <linux/iio/iio.h>
> 
> > +
> > +static int adp810_measure(struct adp810_data *data, struct adp810_read_buf *buf)
> > +{
> > +	struct i2c_client *client = data->client;
> > +	struct device *dev = &client->dev;
> > +	int ret;
> > +	u16 trig_cmd = ADP810_TRIGGER_COMMAND;
> > +
> > +	/* Send trigger to the sensor for measurement */
> > +	ret = i2c_master_send(client, (char *)&trig_cmd, sizeof(u16));
> 
> sizeof(trig_cmd)
> 
> I think it is vanishingly unlikely to matter but in theory i2c_master_send()
> could return 1 and only one byte made it to device.
> So it's common to have 
> 	if (ret < 0)...
> 		....
> 	if (ret != sizeof(trig_cmd))
> 		....
> 

Agree. This is a good corner case. Handled as per the suggestion above.

> > +	if (ret < 0) {
> > +		dev_err(dev, "Error sending trigger command\n");
> > +		return ret;
> > +	}
> > +
> > +	/*
> > +	 * Wait for the sensor to acquire data. As per datasheet section 5.3.1,
> > +	 * wait for at least 10ms before reading measurements from the sensor.
> > +	 */
> > +	msleep(ADP810_MEASURE_LATENCY_MS);
> > +
> > +	/* Read sensor values */
> > +	ret = i2c_master_recv(client, (char *)buf, sizeof(*buf));
> > +	if (ret < 0) {
> 
> Same potential issue for short reads as for the write above.
> 
> I don't recall seeing anything to say we either got full length or
> error code but maybe that changed at somepoint to make this interface easier to use.

ACK. Did similar implementation as _send() for error handling.

> 
> 
> > +		dev_err(dev, "Error reading from sensor\n");
> > +		return ret;
> > +	}
> > +
> > +	/* CRC checks */
> > +	crc8_populate_msb(crc_table, ADP810_CRC8_POLYNOMIAL);
> > +	if (buf->dp_crc != crc8(crc_table, (u8 *)&buf->dp, 0x2, CRC8_INIT_VALUE)) {
> > +		dev_err(dev, "CRC error for pressure\n");
> > +		return -EIO;
> > +	}
> > +
> > +	if (buf->tmp_crc != crc8(crc_table, (u8 *)&buf->tmp, 0x2, CRC8_INIT_VALUE)) {
> > +		dev_err(dev, "CRC error for temperature\n");
> > +		return -EIO;
> > +	}
> > +
> > +	if (buf->sf_crc != crc8(crc_table, (u8 *)&buf->sf, 0x2, CRC8_INIT_VALUE)) {
> > +		dev_err(dev, "CRC error for scale\n");
> > +		return -EIO;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int adp810_read_raw(struct iio_dev *indio_dev,
> > +			   struct iio_chan_spec const *chan,
> > +			   int *val, int *val2, long mask)
> > +{
> > +	struct adp810_data *data = iio_priv(indio_dev);
> > +	struct device *dev = &data->client->dev;
> > +	struct adp810_read_buf buf = {0};
> 
> Not a big thing but slight preference for { }.

Sure. Will use { } insteaed of {0}.

> It's a messy thing wrt to the c spec which never talked about holes
> and this construct until recently.  However the kernel has build tests
> that verify that all compilers will zero the holes with the { }
> syntax and that is what the C standards folk have put in the spec now
> as meaning whole structure including holes.

Good to know the background of this. Thanks.

> 
> > +	int ret;
> > +
> > +	scoped_guard(mutex, &data->lock) {
> > +		ret = adp810_measure(data, &buf);
> > +		if (ret) {
> > +			dev_err(dev, "Failed to read from device\n");
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	switch (mask) {
> 

Re: [PATCH v2 2/2] iio: pressure: adp810: Add driver for adp810 sensor

[Jonathan Cameron]

On Tue, 21 Oct 2025 11:15:42 +0530
Akhilesh Patil <akhilesh@ee.iitb.ac.in> wrote:

> On Sat, Oct 18, 2025 at 05:47:46PM +0100, Jonathan Cameron wrote:
> > On Mon, 13 Oct 2025 22:32:35 +0530
> > Akhilesh Patil <akhilesh@ee.iitb.ac.in> wrote:
> >   
> > > Add driver for Aosong adp810 differential pressure and temperature sensor.
> > > This sensor provides an I2C interface for reading data.
> > > Calculate CRC of the data received using standard crc8 library to verify
> > > data integrity.
> > > 
> > > Tested on TI am62x sk board with sensor connected at i2c-2.
> > > 
> > > Signed-off-by: Akhilesh Patil <akhilesh@ee.iitb.ac.in>  
> > 
> > A few comments inline and it seems your rebase when wrong and you've
> > picked up unrelated build file changes.
> > 
> > Thanks
> > 
> > Jonathan  
> 
> Hi Jonathan, Thanks for the review, I will share v3 addressing these comments.
> 
> Regards,
> Akhilesh
> 
> >   
> > > diff --git a/drivers/iio/pressure/Makefile b/drivers/iio/pressure/Makefile
> > > index 6482288e07ee..a21443e992b9 100644
> > > --- a/drivers/iio/pressure/Makefile
> > > +++ b/drivers/iio/pressure/Makefile
> > > @@ -5,6 +5,7 @@
> > >  
> > >  # When adding new entries keep the list in alphabetical order
> > >  obj-$(CONFIG_ABP060MG) += abp060mg.o
> > > +obj-$(CONFIG_ADP810) += adp810.o
> > >  obj-$(CONFIG_ROHM_BM1390) += rohm-bm1390.o
> > >  obj-$(CONFIG_BMP280) += bmp280.o
> > >  bmp280-objs := bmp280-core.o bmp280-regmap.o
> > > @@ -15,6 +16,7 @@ obj-$(CONFIG_DPS310) += dps310.o
> > >  obj-$(CONFIG_IIO_CROS_EC_BARO) += cros_ec_baro.o
> > >  obj-$(CONFIG_HID_SENSOR_PRESS)   += hid-sensor-press.o
> > >  obj-$(CONFIG_HP03) += hp03.o
> > > +obj-$(CONFIG_HP206C) += hp206c.o
> > >  obj-$(CONFIG_HSC030PA) += hsc030pa.o
> > >  obj-$(CONFIG_HSC030PA_I2C) += hsc030pa_i2c.o
> > >  obj-$(CONFIG_HSC030PA_SPI) += hsc030pa_spi.o
> > > @@ -34,11 +36,9 @@ obj-$(CONFIG_SDP500) += sdp500.o
> > >  obj-$(CONFIG_IIO_ST_PRESS) += st_pressure.o
> > >  st_pressure-y := st_pressure_core.o
> > >  st_pressure-$(CONFIG_IIO_BUFFER) += st_pressure_buffer.o
> > > +obj-$(CONFIG_IIO_ST_PRESS_I2C) += st_pressure_i2c.o
> > > +obj-$(CONFIG_IIO_ST_PRESS_SPI) += st_pressure_spi.o
> > >  obj-$(CONFIG_T5403) += t5403.o
> > > -obj-$(CONFIG_HP206C) += hp206c.o  
> > 
> > Rebase gone wrong I assume.    
> 
> These are intentional changes.
> 
> This addresses Andy's suggestion in v1, to keep entries alphabetically
> arranged in Makefile. Along with adp810 location, fixed other files as well
> hp206 and st_pressure_* to make entries alphabetically arranged in
> the entire Makefile.

That reorder of others needs to be a separate patch so that it can
explicitly call out that it is tidying up ordering.

Jonathan