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

[PATCH 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.

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                 |   1 +
 drivers/iio/pressure/adp810.c                 | 205 ++++++++++++++++++
 5 files changed, 271 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/pressure/aosong,adp810.yaml
 create mode 100644 drivers/iio/pressure/adp810.c

-- 
2.34.1

[PATCH 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>
---
 .../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..540f94b3b10e
--- /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 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

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

[Krzysztof Kozlowski]

On 11/10/2025 14:25, Akhilesh Patil wrote:
> +---
> +$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: |

If there is going to be any new version, drop |.


> +  ADP810 is differential pressure and temperature sensor. It has I2C bus interface

Wrap at 80 (see coding style).



Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

<form letter>
This is an automated instruction, just in case, because many review tags
are being ignored. If you know the process, just skip it entirely
(please do not feel offended by me posting it here - no bad intentions
intended, no patronizing, I just want to avoid wasted efforts). If you
do not know the process, here is a short explanation:

Please add Acked-by/Reviewed-by/Tested-by tags when posting new versions
of patchset, under or above your Signed-off-by tag, unless patch changed
significantly (e.g. new properties added to the DT bindings). Tag is
"received", when provided in a message replied to you on the mailing
list. Tools like b4 can help here ('b4 trailers -u ...'). However,
there's no need to repost patches *only* to add the tags. The upstream
maintainer will do that for tags received on the version they apply.

Full context and explanation:
https://elixir.bootlin.com/linux/v6.15/source/Documentation/process/submitting-patches.rst#L591
</form letter>


Best regards,
Krzysztof

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

[Akhilesh Patil]

On Sun, Oct 12, 2025 at 05:13:53AM +0200, Krzysztof Kozlowski wrote:
> On 11/10/2025 14:25, Akhilesh Patil wrote:
> > +---
> > +$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: |
> 
> If there is going to be any new version, drop |.

Sure. Fixed in v2.

> 
> 
> > +  ADP810 is differential pressure and temperature sensor. It has I2C bus interface
> 
> Wrap at 80 (see coding style).

Fixed.

> 
> 
> 
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> 
> Best regards,
> Krzysztof

Hi Krzysztof, Thanks for the review.

Regards,
Akhilesh

[PATCH 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 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 |   1 +
 drivers/iio/pressure/adp810.c | 205 ++++++++++++++++++++++++++++++++++
 4 files changed, 225 insertions(+)
 create mode 100644 drivers/iio/pressure/adp810.c

diff --git a/MAINTAINERS b/MAINTAINERS
index e4886604631d..52546cb3dac6 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..648da292f8de 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 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..08f5fa724491 100644
--- a/drivers/iio/pressure/Makefile
+++ b/drivers/iio/pressure/Makefile
@@ -42,3 +42,4 @@ 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
+obj-$(CONFIG_ADP810) += adp810.o
diff --git a/drivers/iio/pressure/adp810.c b/drivers/iio/pressure/adp810.c
new file mode 100644
index 000000000000..ff73330b34fc
--- /dev/null
+++ b/drivers/iio/pressure/adp810.c
@@ -0,0 +1,205 @@
+// 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/module.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/crc8.h>
+
+/* Time taken in ms by sensor to do measurements after triggering.
+ * As per datahseet, 10ms is sufficient but we define 30 for better margin
+ */
+#define ADP810_MEASURE_LATENCY		30
+/* Trigger command to send to start measurement by the sensor */
+#define ADP810_TRIGGER_COMMAND		0x2d37
+#define ADP810_CRC8_POLYNOMIAL		0x31
+
+DECLARE_CRC8_TABLE(crc_table);
+
+struct adp810_read_buf {
+	u8 dp_msb;
+	u8 dp_lsb;
+	u8 dp_crc;
+	u8 tmp_msb;
+	u8 tmp_lsb;
+	u8 tmp_crc;
+	u8 sf_msb;
+	u8 sf_lsb;
+	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;
+	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(&client->dev, "Error sending trigger command\n");
+		return ret;
+	}
+
+	/* Wait for sensor to aquire data */
+	msleep(ADP810_MEASURE_LATENCY);
+
+	/* Read sensor values */
+	ret = i2c_master_recv(client, (char *)buf, sizeof(*buf));
+	if (ret < 0) {
+		dev_err(&client->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, &buf->dp_msb, 0x2, CRC8_INIT_VALUE)) {
+		dev_err(&client->dev, "CRC error for pressure\n");
+		return -EIO;
+	}
+
+	if (buf->tmp_crc != crc8(crc_table, &buf->tmp_msb, 0x2, CRC8_INIT_VALUE)) {
+		dev_err(&client->dev, "CRC error for temperature\n");
+		return -EIO;
+	}
+
+	if (buf->sf_crc != crc8(crc_table, &buf->sf_msb, 0x2, CRC8_INIT_VALUE)) {
+		dev_err(&client->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 adp810_read_buf buf = {0};
+	int ret;
+
+	mutex_lock(&data->lock);
+	ret = adp810_measure(data, &buf);
+	mutex_unlock(&data->lock);
+
+	if (ret) {
+		dev_err(&indio_dev->dev, "Failed to read from device\n");
+		return ret;
+	}
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		switch (chan->type) {
+		case IIO_PRESSURE:
+			*val = buf.dp_msb << 8 | buf.dp_lsb;
+			return IIO_VAL_INT;
+		case IIO_TEMP:
+			*val = buf.tmp_msb << 8 | buf.tmp_lsb;
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->type) {
+		case IIO_PRESSURE:
+			*val = buf.sf_msb << 8 | buf.sf_lsb;
+			return IIO_VAL_INT;
+		case IIO_TEMP:
+			*val = 200;
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+	default:
+		return -EINVAL;
+	}
+
+	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)
+{
+	const struct i2c_device_id *dev_id = i2c_client_get_device_id(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;
+	mutex_init(&data->lock);
+
+	indio_dev->name = dev_id->name;
+	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 ret;
+}
+
+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 2/2] iio: pressure: adp810: Add driver for adp810 sensor

[Andy Shevchenko]

On Sat, Oct 11, 2025 at 3:25 PM Akhilesh Patil <akhilesh@ee.iitb.ac.in> wrote:
>
> Add driver for Aosong adp810 differential pressure and
> temperature sensor. This sensor provides I2C interface for

an I²C

> 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

Missing period at the end.

...

> +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

Some tools will report an orphaned yaml file if you apply patch 1
without patch 2.

...

> +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 I2C interface for data communication.

Same as in the commit message.

> +         To compile this driver as a module, choose M here: the module will
> +         be called adp810

...

>  obj-$(CONFIG_IIO_ST_PRESS_I2C) += st_pressure_i2c.o
>  obj-$(CONFIG_IIO_ST_PRESS_SPI) += st_pressure_spi.o
> +obj-$(CONFIG_ADP810) += adp810.o

Is Makefile ordered in terms of files and/or configuration options?


> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/crc8.h>

Please,
1) keep it alphabetically ordered;
2) follow the IWYU (Include What You Use) principle.

...

> +/* Time taken in ms by sensor to do measurements after triggering.

/*
 * Wrong multi-line comment format. You
 * may use this as a reference.
 */

> + * As per datahseet, 10ms is sufficient but we define 30 for better margin

datasheet

Please, respect grammar and punctuation, i.e. again as in the commit
message you made a mistake.

...

> +#define ADP810_MEASURE_LATENCY         30

What's the unit of this value?

...

This needs a comment to explain the choice of this. E.g., reference to
the Datasheet section / chapter.

> +#define ADP810_CRC8_POLYNOMIAL         0x31

...

> +struct adp810_read_buf {
> +       u8 dp_msb;
> +       u8 dp_lsb;
> +       u8 dp_crc;
> +       u8 tmp_msb;
> +       u8 tmp_lsb;
> +       u8 tmp_crc;
> +       u8 sf_msb;
> +       u8 sf_lsb;
> +       u8 sf_crc;
> +} __packed;

Why __packed?

...

> +struct adp810_data {
> +       struct i2c_client *client;
> +       /* Use lock to synchronize access to device during read sequence */
> +       struct mutex lock;
> +};

Is there a deliberate choice to not use regmap I²C APIs?

...

> +       /* Wait for sensor to aquire data */

Spell-check. Also the comment is semi-useless, add the reference to
the datasheet or even cite a part of it to explain this.

> +       msleep(ADP810_MEASURE_LATENCY);

...

> +       mutex_lock(&data->lock);
> +       ret = adp810_measure(data, &buf);
> +       mutex_unlock(&data->lock);
> +
> +       if (ret) {
> +               dev_err(&indio_dev->dev, "Failed to read from device\n");
> +               return ret;
> +       }

Instead, include cleanup,h and use scoped_guard() (and possible
guard()() in some other places, but first answer why not regmap).

...

> +       case IIO_CHAN_INFO_RAW:
> +               switch (chan->type) {
> +               case IIO_PRESSURE:
> +                       *val = buf.dp_msb << 8 | buf.dp_lsb;

Those have to be properly defined to begin with, i.e. __be16. With
that done, use existing conversion helpers from asm/byteorder.h.

> +                       return IIO_VAL_INT;
> +               case IIO_TEMP:
> +                       *val = buf.tmp_msb << 8 | buf.tmp_lsb;

Ditto and so on...

> +                       return IIO_VAL_INT;
> +               default:
> +                       return -EINVAL;
> +               }

...

> +       default:
> +               return -EINVAL;
> +       }
> +
> +       return -EINVAL;

Why is dead code required?

...

> +       mutex_init(&data->lock);

devm

-- 
With Best Regards,
Andy Shevchenko

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

[Krzysztof Kozlowski]

On 11/10/2025 16:10, Andy Shevchenko wrote:
> On Sat, Oct 11, 2025 at 3:25 PM Akhilesh Patil <akhilesh@ee.iitb.ac.in> wrote:
>> +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
> 
> Some tools will report an orphaned yaml file if you apply patch 1
> without patch 2.

You mean checkpatch? That warning is not really relevant. Adding
maintainers entry here for both files is perfectly fine and correct.

Best regards,
Krzysztof

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

[Andy Shevchenko]

On Sun, Oct 12, 2025 at 05:12:26AM +0200, Krzysztof Kozlowski wrote:
> On 11/10/2025 16:10, Andy Shevchenko wrote:
> > On Sat, Oct 11, 2025 at 3:25 PM Akhilesh Patil <akhilesh@ee.iitb.ac.in> wrote:
> >> +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
> > 
> > Some tools will report an orphaned yaml file if you apply patch 1
> > without patch 2.
> 
> You mean checkpatch? That warning is not really relevant. Adding
> maintainers entry here for both files is perfectly fine and correct.

It's relevant as long as I see (false positive) warnings from it. Can somebody
shut the checkpatch up about missing DT files in the MAINTAINERS?

-- 
With Best Regards,
Andy Shevchenko

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

[Krzysztof Kozlowski]

On 23/10/2025 20:51, Andy Shevchenko wrote:
> On Sun, Oct 12, 2025 at 05:12:26AM +0200, Krzysztof Kozlowski wrote:
>> On 11/10/2025 16:10, Andy Shevchenko wrote:
>>> On Sat, Oct 11, 2025 at 3:25 PM Akhilesh Patil <akhilesh@ee.iitb.ac.in> wrote:
>>>> +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
>>>
>>> Some tools will report an orphaned yaml file if you apply patch 1
>>> without patch 2.
>>
>> You mean checkpatch? That warning is not really relevant. Adding
>> maintainers entry here for both files is perfectly fine and correct.
> 
> It's relevant as long as I see (false positive) warnings from it. Can somebody


No, it is not relevant. Just because tool is inefficient does not allow
you to point such nitpicks. You as reviewer are supposed to find
difference which checkpatch warnings are important and which are not and
DO NOT bother contributors with useless points that there is some
orphaned file according to checkpatch.


> shut the checkpatch up about missing DT files in the MAINTAINERS?

That would be great but, if no one does it your comments on "orphaned
file" are counter productive.

Best regards,
Krzysztof

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

[Andy Shevchenko]

On Fri, Oct 24, 2025 at 08:18:21AM +0200, Krzysztof Kozlowski wrote:
> On 23/10/2025 20:51, Andy Shevchenko wrote:
> > On Sun, Oct 12, 2025 at 05:12:26AM +0200, Krzysztof Kozlowski wrote:
> >> On 11/10/2025 16:10, Andy Shevchenko wrote:
> >>> On Sat, Oct 11, 2025 at 3:25 PM Akhilesh Patil <akhilesh@ee.iitb.ac.in> wrote:
> >>>> +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
> >>>
> >>> Some tools will report an orphaned yaml file if you apply patch 1
> >>> without patch 2.
> >>
> >> You mean checkpatch? That warning is not really relevant. Adding
> >> maintainers entry here for both files is perfectly fine and correct.
> > 
> > It's relevant as long as I see (false positive) warnings from it. Can somebody
> 
> 
> No, it is not relevant. Just because tool is inefficient does not allow
> you to point such nitpicks. You as reviewer are supposed to find
> difference which checkpatch warnings are important and which are not and
> DO NOT bother contributors with useless points that there is some
> orphaned file according to checkpatch.
> 
> 
> > shut the checkpatch up about missing DT files in the MAINTAINERS?
> 
> That would be great but, if no one does it your comments on "orphaned
> file" are counter productive.

Something like this?

diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
index 6729f18e5654..818b49d314ce 100755
--- a/scripts/checkpatch.pl
+++ b/scripts/checkpatch.pl
@@ -3441,11 +3441,17 @@ sub process {
 		     ($line =~ /\{\s*([\w\/\.\-]*)\s*\=\>\s*([\w\/\.\-]*)\s*\}/ &&
 		      (defined($1) || defined($2))))) {
 			$is_patch = 1;
-			$reported_maintainer_file = 1;
-			WARN("FILE_PATH_CHANGES",
-			     "added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr);
+			# DT bindings are incorporate maintainer information, no need to report
+			if ($realfile !~ m@^Documentation/devicetree/bindings/@)) {
+				$reported_maintainer_file = 1;
+				WARN("FILE_PATH_CHANGES",
+				     "added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr);
+			}
 		}
 
+		    ($realfile =~ m@^Documentation/devicetree/bindings/.*\.txt$@)) {
+			if ($realfile =~ m@^include/asm/@) {
+
 # Check for adding new DT bindings not in schema format
 		if (!$in_commit_log &&
 		    ($line =~ /^new file mode\s*\d+\s*$/) &&

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Fri, Oct 24, 2025 at 11:34:37AM +0300, Andy Shevchenko wrote:
> On Fri, Oct 24, 2025 at 08:18:21AM +0200, Krzysztof Kozlowski wrote:
> > On 23/10/2025 20:51, Andy Shevchenko wrote:
> > > On Sun, Oct 12, 2025 at 05:12:26AM +0200, Krzysztof Kozlowski wrote:
> > >> On 11/10/2025 16:10, Andy Shevchenko wrote:
> > >>> On Sat, Oct 11, 2025 at 3:25 PM Akhilesh Patil <akhilesh@ee.iitb.ac.in> wrote:

...

> > >>>> +F:     Documentation/devicetree/bindings/iio/pressure/aosong,adp810.yaml
> > >>>> +F:     drivers/iio/pressure/adp810.c
> > >>>
> > >>> Some tools will report an orphaned yaml file if you apply patch 1
> > >>> without patch 2.
> > >>
> > >> You mean checkpatch? That warning is not really relevant. Adding
> > >> maintainers entry here for both files is perfectly fine and correct.
> > > 
> > > It's relevant as long as I see (false positive) warnings from it. Can somebody
> > 
> > No, it is not relevant. Just because tool is inefficient does not allow
> > you to point such nitpicks. You as reviewer are supposed to find
> > difference which checkpatch warnings are important and which are not and
> > DO NOT bother contributors with useless points that there is some
> > orphaned file according to checkpatch.
> > 
> > > shut the checkpatch up about missing DT files in the MAINTAINERS?
> > 
> > That would be great but, if no one does it your comments on "orphaned
> > file" are counter productive.
> 
> Something like this?
> 
> diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> index 6729f18e5654..818b49d314ce 100755
> --- a/scripts/checkpatch.pl
> +++ b/scripts/checkpatch.pl
> @@ -3441,11 +3441,17 @@ sub process {
>  		     ($line =~ /\{\s*([\w\/\.\-]*)\s*\=\>\s*([\w\/\.\-]*)\s*\}/ &&
>  		      (defined($1) || defined($2))))) {
>  			$is_patch = 1;
> -			$reported_maintainer_file = 1;
> -			WARN("FILE_PATH_CHANGES",
> -			     "added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr);
> +			# DT bindings are incorporate maintainer information, no need to report
> +			if ($realfile !~ m@^Documentation/devicetree/bindings/@)) {
> +				$reported_maintainer_file = 1;
> +				WARN("FILE_PATH_CHANGES",
> +				     "added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr);
> +			}
>  		}

> +		    ($realfile =~ m@^Documentation/devicetree/bindings/.*\.txt$@)) {
> +			if ($realfile =~ m@^include/asm/@) {

These two lines are leftovers that needs to be removed, of course.

Akhilesh, can you give a try of this change and see if the original DT schema
binding patch is not reported anymore?

>  # Check for adding new DT bindings not in schema format
>  		if (!$in_commit_log &&
>  		    ($line =~ /^new file mode\s*\d+\s*$/) &&

-- 
With Best Regards,
Andy Shevchenko

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

[Akhilesh Patil]

On Fri, Oct 24, 2025 at 11:37:01AM +0300, Andy Shevchenko wrote:
> On Fri, Oct 24, 2025 at 11:34:37AM +0300, Andy Shevchenko wrote:
> > On Fri, Oct 24, 2025 at 08:18:21AM +0200, Krzysztof Kozlowski wrote:
> > > On 23/10/2025 20:51, Andy Shevchenko wrote:
> > > > On Sun, Oct 12, 2025 at 05:12:26AM +0200, Krzysztof Kozlowski wrote:
> > > >> On 11/10/2025 16:10, Andy Shevchenko wrote:
> > > >>> On Sat, Oct 11, 2025 at 3:25 PM Akhilesh Patil <akhilesh@ee.iitb.ac.in> wrote:
> 
> ...
> 
> > > >>>> +F:     Documentation/devicetree/bindings/iio/pressure/aosong,adp810.yaml
> > > >>>> +F:     drivers/iio/pressure/adp810.c
> > > >>>
> > > >>> Some tools will report an orphaned yaml file if you apply patch 1
> > > >>> without patch 2.
> > > >>
> > > >> You mean checkpatch? That warning is not really relevant. Adding
> > > >> maintainers entry here for both files is perfectly fine and correct.
> > > > 
> > > > It's relevant as long as I see (false positive) warnings from it. Can somebody
> > > 
> > > No, it is not relevant. Just because tool is inefficient does not allow
> > > you to point such nitpicks. You as reviewer are supposed to find
> > > difference which checkpatch warnings are important and which are not and
> > > DO NOT bother contributors with useless points that there is some
> > > orphaned file according to checkpatch.
> > > 
> > > > shut the checkpatch up about missing DT files in the MAINTAINERS?
> > > 
> > > That would be great but, if no one does it your comments on "orphaned
> > > file" are counter productive.
> > 
> > Something like this?
> > 
> > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > index 6729f18e5654..818b49d314ce 100755
> > --- a/scripts/checkpatch.pl
> > +++ b/scripts/checkpatch.pl
> > @@ -3441,11 +3441,17 @@ sub process {
> >  		     ($line =~ /\{\s*([\w\/\.\-]*)\s*\=\>\s*([\w\/\.\-]*)\s*\}/ &&
> >  		      (defined($1) || defined($2))))) {
> >  			$is_patch = 1;
> > -			$reported_maintainer_file = 1;
> > -			WARN("FILE_PATH_CHANGES",
> > -			     "added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr);
> > +			# DT bindings are incorporate maintainer information, no need to report
> > +			if ($realfile !~ m@^Documentation/devicetree/bindings/@)) {
> > +				$reported_maintainer_file = 1;
> > +				WARN("FILE_PATH_CHANGES",
> > +				     "added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr);
> > +			}
> >  		}
> 
> > +		    ($realfile =~ m@^Documentation/devicetree/bindings/.*\.txt$@)) {
> > +			if ($realfile =~ m@^include/asm/@) {
> 
> These two lines are leftovers that needs to be removed, of course.
> 
> Akhilesh, can you give a try of this change and see if the original DT schema
> binding patch is not reported anymore?

Hi Andy. I tested checkpatch.pl patch you suggested here. checkpatch
does NOT show the warning now on my dt-bindings patch. Thanks for
initiating this script improvement.
I believe this is kernel wide script improvement and best to take
independently if I understood correctly.

Regards,
Akhilesh

> 
> >  # Check for adding new DT bindings not in schema format
> >  		if (!$in_commit_log &&
> >  		    ($line =~ /^new file mode\s*\d+\s*$/) &&
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 

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

[Andy Shevchenko]

On Fri, Oct 24, 2025 at 11:20:10PM +0530, Akhilesh Patil wrote:
> On Fri, Oct 24, 2025 at 11:37:01AM +0300, Andy Shevchenko wrote:
> > On Fri, Oct 24, 2025 at 11:34:37AM +0300, Andy Shevchenko wrote:
> > > On Fri, Oct 24, 2025 at 08:18:21AM +0200, Krzysztof Kozlowski wrote:
> > > > On 23/10/2025 20:51, Andy Shevchenko wrote:
> > > > > On Sun, Oct 12, 2025 at 05:12:26AM +0200, Krzysztof Kozlowski wrote:
> > > > >> On 11/10/2025 16:10, Andy Shevchenko wrote:
> > > > >>> On Sat, Oct 11, 2025 at 3:25 PM Akhilesh Patil <akhilesh@ee.iitb.ac.in> wrote:

...

> > > > >>>> +F:     Documentation/devicetree/bindings/iio/pressure/aosong,adp810.yaml
> > > > >>>> +F:     drivers/iio/pressure/adp810.c
> > > > >>>
> > > > >>> Some tools will report an orphaned yaml file if you apply patch 1
> > > > >>> without patch 2.
> > > > >>
> > > > >> You mean checkpatch? That warning is not really relevant. Adding
> > > > >> maintainers entry here for both files is perfectly fine and correct.
> > > > > 
> > > > > It's relevant as long as I see (false positive) warnings from it. Can somebody
> > > > 
> > > > No, it is not relevant. Just because tool is inefficient does not allow
> > > > you to point such nitpicks. You as reviewer are supposed to find
> > > > difference which checkpatch warnings are important and which are not and
> > > > DO NOT bother contributors with useless points that there is some
> > > > orphaned file according to checkpatch.
> > > > 
> > > > > shut the checkpatch up about missing DT files in the MAINTAINERS?
> > > > 
> > > > That would be great but, if no one does it your comments on "orphaned
> > > > file" are counter productive.
> > > 
> > > Something like this?
> > > 
> > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > > index 6729f18e5654..818b49d314ce 100755
> > > --- a/scripts/checkpatch.pl
> > > +++ b/scripts/checkpatch.pl
> > > @@ -3441,11 +3441,17 @@ sub process {
> > >  		     ($line =~ /\{\s*([\w\/\.\-]*)\s*\=\>\s*([\w\/\.\-]*)\s*\}/ &&
> > >  		      (defined($1) || defined($2))))) {
> > >  			$is_patch = 1;
> > > -			$reported_maintainer_file = 1;
> > > -			WARN("FILE_PATH_CHANGES",
> > > -			     "added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr);
> > > +			# DT bindings are incorporate maintainer information, no need to report
> > > +			if ($realfile !~ m@^Documentation/devicetree/bindings/@)) {
> > > +				$reported_maintainer_file = 1;
> > > +				WARN("FILE_PATH_CHANGES",
> > > +				     "added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr);
> > > +			}
> > >  		}
> > 
> > > +		    ($realfile =~ m@^Documentation/devicetree/bindings/.*\.txt$@)) {
> > > +			if ($realfile =~ m@^include/asm/@) {
> > 
> > These two lines are leftovers that needs to be removed, of course.
> > 
> > Akhilesh, can you give a try of this change and see if the original DT schema
> > binding patch is not reported anymore?
> 
> Hi Andy. I tested checkpatch.pl patch you suggested here. checkpatch
> does NOT show the warning now on my dt-bindings patch. Thanks for
> initiating this script improvement.
> I believe this is kernel wide script improvement and best to take
> independently if I understood correctly.

Definitely. May I use your Tested-by tag?

> > >  # Check for adding new DT bindings not in schema format
> > >  		if (!$in_commit_log &&
> > >  		    ($line =~ /^new file mode\s*\d+\s*$/) &&

-- 
With Best Regards,
Andy Shevchenko

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

[Akhilesh Patil]

On Mon, Oct 27, 2025 at 10:24:13AM +0200, Andy Shevchenko wrote:
> On Fri, Oct 24, 2025 at 11:20:10PM +0530, Akhilesh Patil wrote:
> > On Fri, Oct 24, 2025 at 11:37:01AM +0300, Andy Shevchenko wrote:
> > > On Fri, Oct 24, 2025 at 11:34:37AM +0300, Andy Shevchenko wrote:
> > > > On Fri, Oct 24, 2025 at 08:18:21AM +0200, Krzysztof Kozlowski wrote:
> > > > > On 23/10/2025 20:51, Andy Shevchenko wrote:
> > > > > > On Sun, Oct 12, 2025 at 05:12:26AM +0200, Krzysztof Kozlowski wrote:
> > > > > >> On 11/10/2025 16:10, Andy Shevchenko wrote:
> > > > > >>> On Sat, Oct 11, 2025 at 3:25 PM Akhilesh Patil <akhilesh@ee.iitb.ac.in> wrote:
> 
> ...
> 
> > > > > >>>> +F:     Documentation/devicetree/bindings/iio/pressure/aosong,adp810.yaml
> > > > > >>>> +F:     drivers/iio/pressure/adp810.c
> > > > > >>>
> > > > > >>> Some tools will report an orphaned yaml file if you apply patch 1
> > > > > >>> without patch 2.
> > > > > >>
> > > > > >> You mean checkpatch? That warning is not really relevant. Adding
> > > > > >> maintainers entry here for both files is perfectly fine and correct.
> > > > > > 
> > > > > > It's relevant as long as I see (false positive) warnings from it. Can somebody
> > > > > 
> > > > > No, it is not relevant. Just because tool is inefficient does not allow
> > > > > you to point such nitpicks. You as reviewer are supposed to find
> > > > > difference which checkpatch warnings are important and which are not and
> > > > > DO NOT bother contributors with useless points that there is some
> > > > > orphaned file according to checkpatch.
> > > > > 
> > > > > > shut the checkpatch up about missing DT files in the MAINTAINERS?
> > > > > 
> > > > > That would be great but, if no one does it your comments on "orphaned
> > > > > file" are counter productive.
> > > > 
> > > > Something like this?
> > > > 
> > > > diff --git a/scripts/checkpatch.pl b/scripts/checkpatch.pl
> > > > index 6729f18e5654..818b49d314ce 100755
> > > > --- a/scripts/checkpatch.pl
> > > > +++ b/scripts/checkpatch.pl
> > > > @@ -3441,11 +3441,17 @@ sub process {
> > > >  		     ($line =~ /\{\s*([\w\/\.\-]*)\s*\=\>\s*([\w\/\.\-]*)\s*\}/ &&
> > > >  		      (defined($1) || defined($2))))) {
> > > >  			$is_patch = 1;
> > > > -			$reported_maintainer_file = 1;
> > > > -			WARN("FILE_PATH_CHANGES",
> > > > -			     "added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr);
> > > > +			# DT bindings are incorporate maintainer information, no need to report
> > > > +			if ($realfile !~ m@^Documentation/devicetree/bindings/@)) {
> > > > +				$reported_maintainer_file = 1;
> > > > +				WARN("FILE_PATH_CHANGES",
> > > > +				     "added, moved or deleted file(s), does MAINTAINERS need updating?\n" . $herecurr);
> > > > +			}
> > > >  		}
> > > 
> > > > +		    ($realfile =~ m@^Documentation/devicetree/bindings/.*\.txt$@)) {
> > > > +			if ($realfile =~ m@^include/asm/@) {
> > > 
> > > These two lines are leftovers that needs to be removed, of course.
> > > 
> > > Akhilesh, can you give a try of this change and see if the original DT schema
> > > binding patch is not reported anymore?
> > 
> > Hi Andy. I tested checkpatch.pl patch you suggested here. checkpatch
> > does NOT show the warning now on my dt-bindings patch. Thanks for
> > initiating this script improvement.
> > I believe this is kernel wide script improvement and best to take
> > independently if I understood correctly.
> 
> Definitely. May I use your Tested-by tag?

Yes. Sure. 

Tested-by: Akhilesh Patil <akhilesh@ee.iitb.ac.in>

anyways, I can ack on your patch with this, once you create it.

Regards,
Akhilesh

> 
> > > >  # Check for adding new DT bindings not in schema format
> > > >  		if (!$in_commit_log &&
> > > >  		    ($line =~ /^new file mode\s*\d+\s*$/) &&
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

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

[Akhilesh Patil]

On Sat, Oct 11, 2025 at 05:10:58PM +0300, Andy Shevchenko wrote:

Hi Andy,
Thank you for your time and valuable feedback.
Addressed all of them with best of my understanding.
Kindly check my comments on "regmap usage" and "__packed" usage
decisions below.

Sharing v2 of the series with these improvements and fixes.

Regards,
Akhilesh

> On Sat, Oct 11, 2025 at 3:25 PM Akhilesh Patil <akhilesh@ee.iitb.ac.in> wrote:
> >
> > Add driver for Aosong adp810 differential pressure and
> > temperature sensor. This sensor provides I2C interface for
> 
> an I²C

Fixed.

> 
> > 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
> 
> Missing period at the end.

Fixed.

> 
> ...
> 
> > +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
> 
> Some tools will report an orphaned yaml file if you apply patch 1
> without patch 2.

ACK. checkpatch.pl did show me the warning.

> 
> ...
> 
> > +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 I2C interface for data communication.
> 
> Same as in the commit message.

Fixed grammer and missing article.

> 
> > +         To compile this driver as a module, choose M here: the module will
> > +         be called adp810
> 
> ...
> 
> >  obj-$(CONFIG_IIO_ST_PRESS_I2C) += st_pressure_i2c.o
> >  obj-$(CONFIG_IIO_ST_PRESS_SPI) += st_pressure_spi.o
> > +obj-$(CONFIG_ADP810) += adp810.o
> 
> Is Makefile ordered in terms of files and/or configuration options?

Sure. Added adp810 at alphabetically correct place.
While at it, I also corrected order of few other enteries.

> 
> 
> > +#include <linux/module.h>
> > +#include <linux/delay.h>
> > +#include <linux/i2c.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +#include <linux/crc8.h>
> 
> Please,
> 1) keep it alphabetically ordered;
> 2) follow the IWYU (Include What You Use) principle.

Sure. Fixed it.

> 
> ...
> 
> > +/* Time taken in ms by sensor to do measurements after triggering.
> 
> /*
>  * Wrong multi-line comment format. You
>  * may use this as a reference.
>  */
> 

Fixed multiline comment format.

> > + * As per datahseet, 10ms is sufficient but we define 30 for better margin
> 
> datasheet
> 
> Please, respect grammar and punctuation, i.e. again as in the commit
> message you made a mistake.

Fixed.

> 
> ...
> 
> > +#define ADP810_MEASURE_LATENCY         30
> 
> What's the unit of this value?

It is milliseconds.
Redefined this macro as ADP810_MEASURE_LATENCY_MS for better clarity.

> 
> ...
> 
> This needs a comment to explain the choice of this. E.g., reference to
> the Datasheet section / chapter.
> 
> > +#define ADP810_CRC8_POLYNOMIAL         0x31

Added reference to the section from datasheet with small explanation.

> 
> ...
> 
> > +struct adp810_read_buf {
> > +       u8 dp_msb;
> > +       u8 dp_lsb;
> > +       u8 dp_crc;
> > +       u8 tmp_msb;
> > +       u8 tmp_lsb;
> > +       u8 tmp_crc;
> > +       u8 sf_msb;
> > +       u8 sf_lsb;
> > +       u8 sf_crc;
> > +} __packed;
> 
> Why __packed?

Yes. This is the structure used as a buffer to store sensor values read.
Each entry in this structure should be contiguous in the memory because
reference of this structure will be passed to i2c_master_recv() to
receive and fill the data.
__packed will avoid any compiler generated paddings in the structure to
force alignments on certain architectures. We do not want these paddings
and want our struct members to be sequentially ordered as shown, with
no padding and size of the struct should also be 9 bytes as only 9 bytes of
data should be read from the sensor as per the specification.

I could have used array here. But I preferred strcture for better
readability of the code as one can easily see what values are expected
from sensor while reading and in which order.

> 
> ...
> 
> > +struct adp810_data {
> > +       struct i2c_client *client;
> > +       /* Use lock to synchronize access to device during read sequence */
> > +       struct mutex lock;
> > +};
> 
> Is there a deliberate choice to not use regmap I²C APIs?

Yes. I explored that possibility. However this sensor follows simple I2C
client protocol. It does not expose the concept of I2C registers. It
does not follow smbus. Specifically, while reading the measurement from
the sensor, we need to only send the device address with read bit on the bus,
and start reading 9 bytes following that. That is, no register address
should be sent. I am not sure if regmap API has some hack to achive
similar because these APIs expect register addresses to read/write which
this sensor does not follow. Hence using raw i2c functions. I also
thought regmap abstraction is not needed here as this sensor has very
limited commands to send and not many command/configurations.

> 
> ...
> 
> > +       /* Wait for sensor to aquire data */
> 
> Spell-check. Also the comment is semi-useless, add the reference to
> the datasheet or even cite a part of it to explain this.

Sure.

> 
> > +       msleep(ADP810_MEASURE_LATENCY);
> 
> ...
> 
> > +       mutex_lock(&data->lock);
> > +       ret = adp810_measure(data, &buf);
> > +       mutex_unlock(&data->lock);
> > +
> > +       if (ret) {
> > +               dev_err(&indio_dev->dev, "Failed to read from device\n");
> > +               return ret;
> > +       }
> 
> Instead, include cleanup,h and use scoped_guard() (and possible
> guard()() in some other places, but first answer why not regmap).

ACK. Used scoped_guard() to cleanly handle resource locks here.

> 
> ...
> 
> > +       case IIO_CHAN_INFO_RAW:
> > +               switch (chan->type) {
> > +               case IIO_PRESSURE:
> > +                       *val = buf.dp_msb << 8 | buf.dp_lsb;
> 
> Those have to be properly defined to begin with, i.e. __be16. With
> that done, use existing conversion helpers from asm/byteorder.h.
> 
> > +                       return IIO_VAL_INT;
> > +               case IIO_TEMP:
> > +                       *val = buf.tmp_msb << 8 | buf.tmp_lsb;
> 
> Ditto and so on...
> 
> > +                       return IIO_VAL_INT;
> > +               default:
> > +                       return -EINVAL;
> > +               }

Agree. We should use standard helpers.
Used be16 along with get_unalinged_be16() to improve this part.

> 
> ...
> 
> > +       default:
> > +               return -EINVAL;
> > +       }
> > +
> > +       return -EINVAL;
> 
> Why is dead code required?

Removed dead code.

> 
> ...
> 
> > +       mutex_init(&data->lock);
> 
> devm

Nice catch.
Fixed this resource leak which can happen upon module unload.
As I do not have .remove callback with distroy mutex in it,
yes this is needed.

> 
> -- 
> With Best Regards,
> Andy Shevchenko

Regards,
Akhilesh

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

[Andy Shevchenko]

On Mon, Oct 13, 2025 at 08:16:14PM +0530, Akhilesh Patil wrote:
> On Sat, Oct 11, 2025 at 05:10:58PM +0300, Andy Shevchenko wrote:
> > On Sat, Oct 11, 2025 at 3:25 PM Akhilesh Patil <akhilesh@ee.iitb.ac.in> wrote:

...

> > > +struct adp810_read_buf {
> > > +       u8 dp_msb;
> > > +       u8 dp_lsb;
> > > +       u8 dp_crc;
> > > +       u8 tmp_msb;
> > > +       u8 tmp_lsb;
> > > +       u8 tmp_crc;
> > > +       u8 sf_msb;
> > > +       u8 sf_lsb;
> > > +       u8 sf_crc;
> > > +} __packed;
> > 
> > Why __packed?
> 
> Yes. This is the structure used as a buffer to store sensor values read.
> Each entry in this structure should be contiguous in the memory because
> reference of this structure will be passed to i2c_master_recv() to
> receive and fill the data.
> __packed will avoid any compiler generated paddings in the structure to
> force alignments on certain architectures. We do not want these paddings
> and want our struct members to be sequentially ordered as shown, with
> no padding and size of the struct should also be 9 bytes as only 9 bytes of
> data should be read from the sensor as per the specification.
> 
> I could have used array here. But I preferred strcture for better
> readability of the code as one can easily see what values are expected
> from sensor while reading and in which order.

Right, but in this form packed only affects the last member size (due to
alignment), in any case since it's HW mandated requirement, perhaps add a
comment. (Since we also going to use __be16 types, the __packed is required
for that to be properly placed in the memory.)


...

> > > +struct adp810_data {
> > > +       struct i2c_client *client;
> > > +       /* Use lock to synchronize access to device during read sequence */
> > > +       struct mutex lock;
> > > +};
> > 
> > Is there a deliberate choice to not use regmap I²C APIs?
> 
> Yes. I explored that possibility. However this sensor follows simple I2C
> client protocol. It does not expose the concept of I2C registers. It
> does not follow smbus. Specifically, while reading the measurement from
> the sensor, we need to only send the device address with read bit on the bus,
> and start reading 9 bytes following that. That is, no register address
> should be sent. I am not sure if regmap API has some hack to achive
> similar because these APIs expect register addresses to read/write which
> this sensor does not follow. Hence using raw i2c functions. I also
> thought regmap abstraction is not needed here as this sensor has very
> limited commands to send and not many command/configurations.

Ah, makes sense.

-- 
With Best Regards,
Andy Shevchenko

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

[Akhilesh Patil]

On Thu, Oct 23, 2025 at 09:58:45PM +0300, Andy Shevchenko wrote:
> On Mon, Oct 13, 2025 at 08:16:14PM +0530, Akhilesh Patil wrote:
> > On Sat, Oct 11, 2025 at 05:10:58PM +0300, Andy Shevchenko wrote:
> > > On Sat, Oct 11, 2025 at 3:25 PM Akhilesh Patil <akhilesh@ee.iitb.ac.in> wrote:
> 
> ...
> 
> > > > +struct adp810_read_buf {
> > > > +       u8 dp_msb;
> > > > +       u8 dp_lsb;
> > > > +       u8 dp_crc;
> > > > +       u8 tmp_msb;
> > > > +       u8 tmp_lsb;
> > > > +       u8 tmp_crc;
> > > > +       u8 sf_msb;
> > > > +       u8 sf_lsb;
> > > > +       u8 sf_crc;
> > > > +} __packed;
> > > 
> > > Why __packed?
> > 
> > Yes. This is the structure used as a buffer to store sensor values read.
> > Each entry in this structure should be contiguous in the memory because
> > reference of this structure will be passed to i2c_master_recv() to
> > receive and fill the data.
> > __packed will avoid any compiler generated paddings in the structure to
> > force alignments on certain architectures. We do not want these paddings
> > and want our struct members to be sequentially ordered as shown, with
> > no padding and size of the struct should also be 9 bytes as only 9 bytes of
> > data should be read from the sensor as per the specification.
> > 
> > I could have used array here. But I preferred strcture for better
> > readability of the code as one can easily see what values are expected
> > from sensor while reading and in which order.
> 
> Right, but in this form packed only affects the last member size (due to
> alignment), in any case since it's HW mandated requirement, perhaps add a
> comment. (Since we also going to use __be16 types, the __packed is required
> for that to be properly placed in the memory.)

Sure. Added comment explaning this in v4.

Regards,
Akhilesh

> 
> 
> ...
> 
> > > > +struct adp810_data {
> > > > +       struct i2c_client *client;
> > > > +       /* Use lock to synchronize access to device during read sequence */
> > > > +       struct mutex lock;
> > > > +};
> > > 
> > > Is there a deliberate choice to not use regmap I²C APIs?
> > 
> > Yes. I explored that possibility. However this sensor follows simple I2C
> > client protocol. It does not expose the concept of I2C registers. It
> > does not follow smbus. Specifically, while reading the measurement from
> > the sensor, we need to only send the device address with read bit on the bus,
> > and start reading 9 bytes following that. That is, no register address
> > should be sent. I am not sure if regmap API has some hack to achive
> > similar because these APIs expect register addresses to read/write which
> > this sensor does not follow. Hence using raw i2c functions. I also
> > thought regmap abstraction is not needed here as this sensor has very
> > limited commands to send and not many command/configurations.
> 
> Ah, makes sense.
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

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

[Jonathan Cameron]

On Sat, 11 Oct 2025 17:55:28 +0530
Akhilesh Patil <akhilesh@ee.iitb.ac.in> wrote:

Hi Akhilesh, 

Thanks for sending this and a late welcome to IIO.

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

> Tested on TI am62x sk board with sensor connected at i2c-2
> 
> Signed-off-by: Akhilesh Patil <akhilesh@ee.iitb.ac.in>

Where I've remembered Andy commenting on something I've not duplicated
(assuming I even noticed the same thing!)

A few things may contradict or provide alternative suggestions though!

Jonathan


> diff --git a/drivers/iio/pressure/adp810.c b/drivers/iio/pressure/adp810.c
> new file mode 100644
> index 000000000000..ff73330b34fc
> --- /dev/null
> +++ b/drivers/iio/pressure/adp810.c
> @@ -0,0 +1,205 @@
> +/* Trigger command to send to start measurement by the sensor */
> +#define ADP810_TRIGGER_COMMAND		0x2d37
> +#define ADP810_CRC8_POLYNOMIAL		0x31
> +
> +DECLARE_CRC8_TABLE(crc_table);
> +
> +struct adp810_read_buf {
> +	u8 dp_msb;
> +	u8 dp_lsb;

__be16 dp;
or u8 dp[2]; 

> +	u8 dp_crc;
> +	u8 tmp_msb;
> +	u8 tmp_lsb;

__be16_tmp;

> +	u8 tmp_crc;
> +	u8 sf_msb;
> +	u8 sf_lsb;

__be16 sf;

> +	u8 sf_crc;
> +} __packed;
With packed (which you didn't need previously).
(more below)

> +
> +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;
> +	int ret;
Not sure what ordering you are using for declarations but this looks a bit
odd.  If nothing else makes more sense go with reverse xmas tree.

> +	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(&client->dev, "Error sending trigger command\n");
> +		return ret;
> +	}
> +
> +	/* Wait for sensor to aquire data */
> +	msleep(ADP810_MEASURE_LATENCY);
> +
> +	/* Read sensor values */
> +	ret = i2c_master_recv(client, (char *)buf, sizeof(*buf));
> +	if (ret < 0) {
> +		dev_err(&client->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, &buf->dp_msb, 0x2, CRC8_INIT_VALUE)) {
> +		dev_err(&client->dev, "CRC error for pressure\n");
> +		return -EIO;
> +	}
> +
> +	if (buf->tmp_crc != crc8(crc_table, &buf->tmp_msb, 0x2, CRC8_INIT_VALUE)) {
> +		dev_err(&client->dev, "CRC error for temperature\n");
> +		return -EIO;
> +	}
> +
> +	if (buf->sf_crc != crc8(crc_table, &buf->sf_msb, 0x2, CRC8_INIT_VALUE)) {
> +		dev_err(&client->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 adp810_read_buf buf = {0};
> +	int ret;
> +
> +	mutex_lock(&data->lock);
> +	ret = adp810_measure(data, &buf);
> +	mutex_unlock(&data->lock);
> +
> +	if (ret) {
> +		dev_err(&indio_dev->dev, "Failed to read from device\n");
It's normally more informative to use the parent dev for error messages.
data->client->dev here.
Probably add a local variable struct device *dev, to avoid making the dev_err() lines
even longer.

> +		return ret;
> +	}
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		switch (chan->type) {
> +		case IIO_PRESSURE:
> +			*val = buf.dp_msb << 8 | buf.dp_lsb;

They are next to each other so either treating them as a small array or
I think you can even make the be16 you can use
			*val = get_unaligned_be16(buf.dp);
for all 3 similar cases.  1st and 3rd are actually aligned but
lets not rely on that.

> +			return IIO_VAL_INT;
> +		case IIO_TEMP:
> +			*val = buf.tmp_msb << 8 | buf.tmp_lsb;
> +			return IIO_VAL_INT;
> +		default:
> +			return -EINVAL;
> +		}
> +	case IIO_CHAN_INFO_SCALE:
> +		switch (chan->type) {
> +		case IIO_PRESSURE:
> +			*val = buf.sf_msb << 8 | buf.sf_lsb;
> +			return IIO_VAL_INT;
> +		case IIO_TEMP:
> +			*val = 200;
> +			return IIO_VAL_INT;
> +		default:
> +			return -EINVAL;
> +		}
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return -EINVAL;
> +}

> +static int adp810_probe(struct i2c_client *client)
> +{
> +	const struct i2c_device_id *dev_id = i2c_client_get_device_id(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;
> +	mutex_init(&data->lock);
Andy mentioned this.  I used not to care about mutex debugging in IIO drivers
as in most cases lifetimes of the containing structure are so closely aligned
to the mutex it wasn't worth the extra debugging provided by mutex_destroy().

Now we can do
	ret = devm_mutex_init(&data->lock);
	if (ret)
		return ret;

It's so easy I would like to see it used in all new code even when the
gain is very small.

> +
> +	indio_dev->name = dev_id->name;

Just set it to "adp810" here.  We can add more complex handling when the driver
supports additional parts.  The advantage of an explicit string for now is
we don't have to think about what it can be.

> +	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 ret;
return 0; 

As that clearly indicates to the reader that you can't get here in an error case.

> +}

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

[Akhilesh Patil]

On Sun, Oct 12, 2025 at 08:36:58PM +0100, Jonathan Cameron wrote:
> On Sat, 11 Oct 2025 17:55:28 +0530
> Akhilesh Patil <akhilesh@ee.iitb.ac.in> wrote:
> 
> Hi Akhilesh, 
> 
> Thanks for sending this and a late welcome to IIO.

Hi Jonathan, Thanks for the review.
Addressing the comments and suggestions in v2.

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

I think it is already wrapped to 75. 
Still, I will recheck and fix if required.

> 
> > Tested on TI am62x sk board with sensor connected at i2c-2
> > 
> > Signed-off-by: Akhilesh Patil <akhilesh@ee.iitb.ac.in>
> 
> Where I've remembered Andy commenting on something I've not duplicated
> (assuming I even noticed the same thing!)
> 
> A few things may contradict or provide alternative suggestions though!
> 
> Jonathan

Okay.

> 
> 
> > diff --git a/drivers/iio/pressure/adp810.c b/drivers/iio/pressure/adp810.c
> > new file mode 100644
> > index 000000000000..ff73330b34fc
> > --- /dev/null
> > +++ b/drivers/iio/pressure/adp810.c
> > @@ -0,0 +1,205 @@
> > +/* Trigger command to send to start measurement by the sensor */
> > +#define ADP810_TRIGGER_COMMAND		0x2d37
> > +#define ADP810_CRC8_POLYNOMIAL		0x31
> > +
> > +DECLARE_CRC8_TABLE(crc_table);
> > +
> > +struct adp810_read_buf {
> > +	u8 dp_msb;
> > +	u8 dp_lsb;
> 
> __be16 dp;
> or u8 dp[2]; 
> 
> > +	u8 dp_crc;
> > +	u8 tmp_msb;
> > +	u8 tmp_lsb;
> 
> __be16_tmp;
> 
> > +	u8 tmp_crc;
> > +	u8 sf_msb;
> > +	u8 sf_lsb;
> 
> __be16 sf;
> 
> > +	u8 sf_crc;
> > +} __packed;
> With packed (which you didn't need previously).
> (more below)

Yes. Used __be16 to indicate big endian.

> 
> > +
> > +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;
> > +	int ret;
> Not sure what ordering you are using for declarations but this looks a bit
> odd.  If nothing else makes more sense go with reverse xmas tree.

okay. I will use "reverse xmas tree" wherever applicable.

> > +	if (buf->tmp_crc != crc8(crc_table, &buf->tmp_msb, 0x2, CRC8_INIT_VALUE)) {
> > +		dev_err(&client->dev, "CRC error for temperature\n");
> > +		return -EIO;
> > +	}
> > +
> > +	if (buf->sf_crc != crc8(crc_table, &buf->sf_msb, 0x2, CRC8_INIT_VALUE)) {
> > +		dev_err(&client->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 adp810_read_buf buf = {0};
> > +	int ret;
> > +
> > +	mutex_lock(&data->lock);
> > +	ret = adp810_measure(data, &buf);
> > +	mutex_unlock(&data->lock);
> > +
> > +	if (ret) {
> > +		dev_err(&indio_dev->dev, "Failed to read from device\n");
> It's normally more informative to use the parent dev for error messages.
> data->client->dev here.
> Probably add a local variable struct device *dev, to avoid making the dev_err() lines
> even longer.

Agree. Will use parent dev in all dev_err() calls.

> 
> > +		return ret;
> > +	}
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		switch (chan->type) {
> > +		case IIO_PRESSURE:
> > +			*val = buf.dp_msb << 8 | buf.dp_lsb;
> 
> They are next to each other so either treating them as a small array or
> I think you can even make the be16 you can use
> 			*val = get_unaligned_be16(buf.dp);
> for all 3 similar cases.  1st and 3rd are actually aligned but
> lets not rely on that.

ACK. Used __be16 along with helpers get_unalinged_be16() to handle
endianess in clean and portable way.

> 
> > +			return IIO_VAL_INT;
> > +		case IIO_TEMP:
> > +			*val = buf.tmp_msb << 8 | buf.tmp_lsb;
> > +			return IIO_VAL_INT;
> > +		default:
> > +			return -EINVAL;
> > +		}
> > +	case IIO_CHAN_INFO_SCALE:
> > +		switch (chan->type) {
> > +		case IIO_PRESSURE:
> > +			*val = buf.sf_msb << 8 | buf.sf_lsb;
> > +			return IIO_VAL_INT;
> > +		case IIO_TEMP:
> 
> > +static int adp810_probe(struct i2c_client *client)
> > +{
> > +	const struct i2c_device_id *dev_id = i2c_client_get_device_id(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;
> > +	mutex_init(&data->lock);
> Andy mentioned this.  I used not to care about mutex debugging in IIO drivers
> as in most cases lifetimes of the containing structure are so closely aligned
> to the mutex it wasn't worth the extra debugging provided by mutex_destroy().
> 
> Now we can do
> 	ret = devm_mutex_init(&data->lock);
> 	if (ret)
> 		return ret;
> 
> It's so easy I would like to see it used in all new code even when the
> gain is very small.

Okay. Using resource managed mutex_init : devm_mutex_init()

> 
> > +
> > +	indio_dev->name = dev_id->name;
> 
> Just set it to "adp810" here.  We can add more complex handling when the driver
> supports additional parts.  The advantage of an explicit string for now is
> we don't have to think about what it can be.
> 

Sure. Directly using string : 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 ret;
> return 0; 
> 
> As that clearly indicates to the reader that you can't get here in an error case.

Yes. Fixed.

Regards,
Akhilesh