iio: pressure: add driver and bindings for adp810
Subsystems (5)
Akhilesh Patil
maintainercover iio: pressure: add driver and bindings for adp810
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
Signature · 1 line
2.34.1
Akhilesh Patil
maintainerPATCH 1/2 dt-bindings: iio: pressure: Add Aosong adp810
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.
--- .../bindings/iio/pressure/aosong,adp810.yaml | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/pressure/aosong,adp810.yaml
Documentation/devicetree/bindings/iio/pressure/aosong,adp810.yaml new file +46 -0
@@ -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 <[email protected]> + +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>; + }; + }; +
Signature · 1 line
2.34.1
Re: [PATCH 1/2] dt-bindings: iio: pressure: Add Aosong adp810
Documentation/devicetree/bindings/iio/pressure/aosong,adp810.yaml line 12
Quoted 10 lines · Akhilesh Patil
+---
+$id: http://devicetree.org/schemas/iio/pressure/aosong,adp810.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#Documentation/devicetree/bindings/iio/pressure/aosong,adp810.yaml new file +7
-0+ +title: aosong adp810 differential pressure sensor + +maintainers: + - Akhilesh Patil <[email protected]> + +description: |
If there is going to be any new version, drop |.
Quoted 1 line
+ ADP810 is differential pressure and temperature sensor. It has I2C bus interface
Wrap at 80 (see coding style).
<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
Akhilesh Patil
maintainerRe: [PATCH 1/2] dt-bindings: iio: pressure: Add Aosong adp810
Quoted 13 lines · Krzysztof Kozlowski
Documentation/devicetree/bindings/iio/pressure/aosong,adp810.yaml line 12
Quoted 10 lines · Akhilesh Patil
+---
+$id: http://devicetree.org/schemas/iio/pressure/aosong,adp810.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#Documentation/devicetree/bindings/iio/pressure/aosong,adp810.yaml new file +7
-0+ +title: aosong adp810 differential pressure sensor + +maintainers: + - Akhilesh Patil <[email protected]> + +description: |If there is going to be any new version, drop |.
Sure. Fixed in v2.
Quoted 5 lines
Quoted 3 lines
+ ADP810 is differential pressure and temperature sensor. It has I2C bus interface
Wrap at 80 (see coding style).
Fixed.
Quoted 7 lines
Reviewed-by Krzysztof Kozlowski <[email protected]>Best regards,
Krzysztof
Hi Krzysztof, Thanks for the review.
Regards,
Akhilesh
Akhilesh Patil
maintainerPATCH 2/2 iio: pressure: adp810: Add driver for adp810 sensor
In reply to Akhilesh PatilAdd 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
--- 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
MAINTAINERS +7 -0
@@ -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 <[email protected]> +L: [email protected] +S: Maintained +F: Documentation/devicetree/bindings/iio/pressure/aosong,adp810.yaml +F: drivers/iio/pressure/adp810.c + ASC7621 HARDWARE MONITOR DRIVER M: George Joseph <[email protected]> L: [email protected]
drivers/iio/pressure/Kconfig +12 -0
@@ -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
drivers/iio/pressure/Makefile +1 -0
@@ -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
drivers/iio/pressure/adp810.c new file +205 -0
205 lines not on this page · raw message
Signature · 1 line
2.34.1
Re: [PATCH 2/2] iio: pressure: adp810: Add driver for adp810 sensor
Quoted 3 lines · Akhilesh Patil
Add driver for Aosong adp810 differential pressure and temperature sensor. This sensor provides I2C interface for
an I²C
Quoted 4 lines
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.
...
MAINTAINERS S: Maintained · line 3753
Quoted 6 lines
MAINTAINERS +6
-0+AOSONG ADP810 DIFFERENTIAL PRESSURE SENSOR DRIVER +M: Akhilesh Patil <[email protected]> +L: [email protected] +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.
...
drivers/iio/pressure/Kconfig config ZPA2326_SPI · line 349
Quoted 8 lines
drivers/iio/pressure/Kconfig +8
-0+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.
Quoted 2 lines
drivers/iio/pressure/Kconfig +2
-0+ To compile this driver as a module, choose M here: the module will + be called adp810
...
Quoted 3 lines
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?
drivers/iio/pressure/adp810.c line 15
Quoted 6 lines
drivers/iio/pressure/adp810.c new file +6
-0+#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.
...
Quoted 1 line
+/* Time taken in ms by sensor to do measurements after triggering.
/*
* Wrong multi-line comment format. You
* may use this as a reference.
*/
Quoted 1 line
+ * 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.
...
Quoted 1 line
+#define
ADP810_MEASURE_LATENCY30
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.
Quoted 1 line
+#define
ADP810_CRC8_POLYNOMIAL0x31
...
drivers/iio/pressure/adp810.c line 37
Quoted 11 lines
drivers/iio/pressure/adp810.c new file +11
-0+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?
...
drivers/iio/pressure/adp810.c line 43
Quoted 5 lines
drivers/iio/pressure/adp810.c new file +5
-0+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?
...
Quoted 1 line
+ /* 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.
Quoted 1 line
+
msleep(ADP810_MEASURE_LATENCY);
...
drivers/iio/pressure/adp810.c line 103
Quoted 8 lines
drivers/iio/pressure/adp810.c new file +8
-0+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).
...
drivers/iio/pressure/adp810.c line 109
Quoted 4 lines
drivers/iio/pressure/adp810.c new file +4
-0+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.
drivers/iio/pressure/adp810.c line 112
Quoted 3 lines
drivers/iio/pressure/adp810.c new file +3
-0+return IIO_VAL_INT ; +case IIO_TEMP : + *val = buf.tmp_msb <<8 | buf.tmp_lsb;
Ditto and so on...
Quoted 4 lines
drivers/iio/pressure/adp810.c new file +4
-0+return IIO_VAL_INT ; +default : +return -EINVAL ; + }
...
drivers/iio/pressure/adp810.c line 132
Quoted 5 lines
drivers/iio/pressure/adp810.c new file +5
-0+default : +return -EINVAL ; + } + +return -EINVAL ;
Why is dead code required?
...
Quoted 1 line
+
mutex_init(&data->lock);
devm
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH 2/2] iio: pressure: adp810: Add driver for adp810 sensor
Quoted 10 lines · Andy Shevchenko
MAINTAINERS S: Maintained · line 3753
Quoted 6 lines · Akhilesh Patil
MAINTAINERS +6
-0+AOSONG ADP810 DIFFERENTIAL PRESSURE SENSOR DRIVER +M: Akhilesh Patil <[email protected]> +L: [email protected] +S: Maintained +F: Documentation/devicetree/bindings/iio/pressure/aosong,adp810.yaml +F: drivers/iio/pressure/adp810.cSome 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
Quoted 12 lines · Krzysztof Kozlowski
Quoted 10 lines · Andy Shevchenko
MAINTAINERS S: Maintained · line 3753
Quoted 6 lines · Akhilesh Patil
MAINTAINERS +6
-0+AOSONG ADP810 DIFFERENTIAL PRESSURE SENSOR DRIVER +M: Akhilesh Patil <[email protected]> +L: [email protected] +S: Maintained +F: Documentation/devicetree/bindings/iio/pressure/aosong,adp810.yaml +F: drivers/iio/pressure/adp810.cSome tools will report an orphaned yaml file if you apply patch 1 without patch 2.
Quoted 2 lines
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?
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH 2/2] iio: pressure: adp810: Add driver for adp810 sensor
Quoted 13 lines · Andy Shevchenko
Quoted 12 lines · Krzysztof Kozlowski
Quoted 10 lines · Andy Shevchenko
MAINTAINERS S: Maintained · line 3753
Quoted 6 lines · Akhilesh Patil
MAINTAINERS +6
-0+AOSONG ADP810 DIFFERENTIAL PRESSURE SENSOR DRIVER +M: Akhilesh Patil <[email protected]> +L: [email protected] +S: Maintained +F: Documentation/devicetree/bindings/iio/pressure/aosong,adp810.yaml +F: drivers/iio/pressure/adp810.cSome tools will report an orphaned yaml file if you apply patch 1 without patch 2.
Quoted 4 lines
Quoted 2 lines
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.
Quoted 1 line
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
Quoted 27 lines · Krzysztof Kozlowski
Quoted 13 lines · Andy Shevchenko
Quoted 12 lines · Krzysztof Kozlowski
Quoted 10 lines · Andy Shevchenko
Review
Quoted 6 lines · Akhilesh Patil
(quoted diff) +6
-0+AOSONG ADP810 DIFFERENTIAL PRESSURE SENSOR DRIVER +M: Akhilesh Patil <[email protected]> +L: [email protected] +S: Maintained +F: Documentation/devicetree/bindings/iio/pressure/aosong,adp810.yaml +F: drivers/iio/pressure/adp810.cSome tools will report an orphaned yaml file if you apply patch 1 without patch 2.
Quoted 4 lines
Quoted 2 lines
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.
Quoted 4 lines
Quoted 1 line
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?
scripts/checkpatch.pl +9 -3
@@ -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*$/) &&
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH 2/2] iio: pressure: adp810: Add driver for adp810 sensor
Quoted 5 lines · Andy Shevchenko
Quoted 4 lines · Krzysztof Kozlowski
Quoted 3 lines · Andy Shevchenko
Quoted 2 lines · Krzysztof Kozlowski
Quoted 1 line · Andy Shevchenko
On Sat, Oct 11, 2025 at 3:25 PM Akhilesh Patil <[email protected]> wrote:
...
Quoted 35 lines
Quoted 17 lines
Quoted 10 lines
Quoted 8 lines
Quoted 5 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+F: Documentation/devicetree/bindings/iio/pressure/aosong,adp810.yaml +F: drivers/iio/pressure/adp810.cSome 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.
Quoted 4 lines
Quoted 1 line
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?
scripts/checkpatch.pl +0
-3@@ -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);
scripts/checkpatch.pl sub process { · line 3453
Quoted 10 lines
scripts/checkpatch.pl +8
-0+ # 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?
Quoted 3 lines
scripts/checkpatch.pl +0
-0# Check for adding new DT bindings not in schema format if (!$in_commit_log && ($line =~ /^new file mode\s*\d+\s*$/) &&
Signature · 2 lines
With Best Regards, Andy Shevchenko
Akhilesh Patil
maintainerRe: [PATCH 2/2] iio: pressure: adp810: Add driver for adp810 sensor
Quoted 52 lines · Andy Shevchenko
Quoted 5 lines · Andy Shevchenko
Quoted 4 lines · Krzysztof Kozlowski
Quoted 3 lines · Andy Shevchenko
Quoted 2 lines · Krzysztof Kozlowski
Quoted 1 line · Andy Shevchenko
On Sat, Oct 11, 2025 at 3:25 PM Akhilesh Patil <[email protected]> wrote:
...
Quoted 42 lines
Quoted 17 lines
Quoted 10 lines
Quoted 8 lines
Quoted 5 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+F: Documentation/devicetree/bindings/iio/pressure/aosong,adp810.yaml +F: drivers/iio/pressure/adp810.cSome 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.
Quoted 4 lines
Quoted 1 line
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?
scripts/checkpatch.pl +6
-3@@ -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); + } }
Quoted 7 lines
scripts/checkpatch.pl sub process { · line 3453
Quoted 2 lines
scripts/checkpatch.pl +2
-0+ ($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
Quoted 9 lines
Quoted 4 lines
scripts/checkpatch.pl +0
-0# Check for adding new DT bindings not in schema format if (!$in_commit_log && ($line =~ /^new file mode\s*\d+\s*$/) &&Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH 2/2] iio: pressure: adp810: Add driver for adp810 sensor
Quoted 7 lines · Akhilesh Patil
Quoted 6 lines · Andy Shevchenko
Quoted 5 lines · Andy Shevchenko
Quoted 4 lines · Krzysztof Kozlowski
Quoted 3 lines · Andy Shevchenko
Quoted 2 lines · Krzysztof Kozlowski
Quoted 1 line · Andy Shevchenko
On Sat, Oct 11, 2025 at 3:25 PM Akhilesh Patil <[email protected]> wrote:
...
Quoted 48 lines
Quoted 48 lines
Quoted 35 lines
Quoted 17 lines
Quoted 10 lines
Quoted 8 lines
Quoted 5 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+F: Documentation/devicetree/bindings/iio/pressure/aosong,adp810.yaml +F: drivers/iio/pressure/adp810.cSome 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.
Quoted 4 lines
Quoted 1 line
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?
scripts/checkpatch.pl +0
-3@@ -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);scripts/checkpatch.pl sub process { · line 3453
Quoted 10 lines
scripts/checkpatch.pl +8
-0+ # 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.
Quoted 8 lines
Quoted 2 lines
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?
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
scripts/checkpatch.pl +0
-0# Check for adding new DT bindings not in schema format if (!$in_commit_log && ($line =~ /^new file mode\s*\d+\s*$/) &&
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH 2/2] iio: pressure: adp810: Add driver for adp810 sensor
Quoted 62 lines · Andy Shevchenko
Quoted 7 lines · Akhilesh Patil
Quoted 6 lines · Andy Shevchenko
Quoted 5 lines · Andy Shevchenko
Quoted 4 lines · Krzysztof Kozlowski
Quoted 3 lines · Andy Shevchenko
Quoted 2 lines · Krzysztof Kozlowski
Quoted 1 line · Andy Shevchenko
On Sat, Oct 11, 2025 at 3:25 PM Akhilesh Patil <[email protected]> wrote:
...
Quoted 51 lines
Quoted 50 lines
Quoted 35 lines
Quoted 17 lines
Quoted 10 lines
Quoted 8 lines
Quoted 5 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+F: Documentation/devicetree/bindings/iio/pressure/aosong,adp810.yaml +F: drivers/iio/pressure/adp810.cSome 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.
Quoted 4 lines
Quoted 1 line
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?
scripts/checkpatch.pl +0
-3@@ -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);scripts/checkpatch.pl sub process { · line 3453
Quoted 10 lines
scripts/checkpatch.pl +8
-0+ # 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?
Quoted 7 lines
Quoted 5 lines
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.
anyways, I can ack on your patch with this, once you create it.
Regards,
Akhilesh
Quoted 10 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
scripts/checkpatch.pl +0
-0# Check for adding new DT bindings not in schema format if (!$in_commit_log && ($line =~ /^new file mode\s*\d+\s*$/) &&Signature · 2 lines
With Best Regards, Andy Shevchenko
Akhilesh Patil
maintainerRe: [PATCH 2/2] iio: pressure: adp810: Add driver for adp810 sensor
In reply to Andy ShevchenkoOn 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
Quoted 6 lines · Akhilesh Patil
Quoted 3 lines
Add driver for Aosong adp810 differential pressure and temperature sensor. This sensor provides I2C interface for
an I²C
Fixed.
Quoted 7 lines
Quoted 5 lines
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.
Quoted 12 lines
...
MAINTAINERS S: Maintained · line 3753
Quoted 6 lines
MAINTAINERS +6
-0+AOSONG ADP810 DIFFERENTIAL PRESSURE SENSOR DRIVER +M: Akhilesh Patil <[email protected]> +L: [email protected] +S: Maintained +F: Documentation/devicetree/bindings/iio/pressure/aosong,adp810.yaml +F: drivers/iio/pressure/adp810.cSome tools will report an orphaned yaml file if you apply patch 1 without patch 2.
ACK. checkpatch.pl did show me the warning.
Quoted 12 lines
...
drivers/iio/pressure/Kconfig config ZPA2326_SPI · line 349
Quoted 8 lines
drivers/iio/pressure/Kconfig +8
-0+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.
Quoted 1 line
Same as in the commit message.
Fixed grammer and missing article.
Quoted 11 lines
Quoted 3 lines
drivers/iio/pressure/Kconfig +2
-0+ To compile this driver as a module, choose M here: the module will + be called adp810...
Quoted 3 lines
obj-$(CONFIG_IIO_ST_PRESS_I2C) += st_pressure_i2c.o obj-$(CONFIG_IIO_ST_PRESS_SPI) += st_pressure_spi.o
+obj-$(CONFIG_ADP810) += adp810.oIs
Makefileordered 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.
Quoted 12 lines
drivers/iio/pressure/adp810.c line 15
Quoted 8 lines
drivers/iio/pressure/adp810.c new file +6
-0+#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.
Quoted 10 lines
...
Quoted 1 line
+/* 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.
Quoted 6 lines
Quoted 1 line
+ * 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.
Quoted 6 lines
...
Quoted 1 line
+#define
ADP810_MEASURE_LATENCY30What's the unit of this value?
It is milliseconds.
Redefined this macro as ADP810_MEASURE_LATENCY_MS for better clarity.
Quoted 7 lines
...
This needs a comment to explain the choice of this. E.g., reference to the Datasheet section / chapter.
Quoted 1 line
+#define
ADP810_CRC8_POLYNOMIAL0x31
Added reference to the section from datasheet with small explanation.
Quoted 16 lines
...
drivers/iio/pressure/adp810.c line 37
Quoted 11 lines
drivers/iio/pressure/adp810.c new file +11
-0+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.
Quoted 10 lines
...
drivers/iio/pressure/adp810.c line 43
Quoted 5 lines
drivers/iio/pressure/adp810.c new file +5
-0+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.
Quoted 7 lines
...
Quoted 1 line
+ /* 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.
Quoted 14 lines
Quoted 2 lines
+
msleep(ADP810_MEASURE_LATENCY);...
drivers/iio/pressure/adp810.c line 103
Quoted 8 lines
drivers/iio/pressure/adp810.c new file +8
-0+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; + }
Quoted 2 lines
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.
Quoted 8 lines
...
drivers/iio/pressure/adp810.c line 109
Quoted 4 lines
drivers/iio/pressure/adp810.c new file +4
-0+case IIO_CHAN_INFO_RAW : +switch (chan->type) { +case IIO_PRESSURE : + *val = buf.dp_msb <<8 | buf.dp_lsb;
Quoted 13 lines
Those have to be properly defined to begin with, i.e.
__be16. With that done, use existing conversion helpers fromasm/byteorder.h.drivers/iio/pressure/adp810.c line 112
Quoted 3 lines
drivers/iio/pressure/adp810.c new file +3
-0+return IIO_VAL_INT ; +case IIO_TEMP : + *val = buf.tmp_msb <<8 | buf.tmp_lsb;Ditto and so on...
Quoted 4 lines
drivers/iio/pressure/adp810.c new file +4
-0+return IIO_VAL_INT ; +default : +return -EINVAL ; + }
Agree. We should use standard helpers.
Used be16 along with get_unalinged_be16() to improve this part.
Quoted 10 lines
...
drivers/iio/pressure/adp810.c line 132
Quoted 5 lines
drivers/iio/pressure/adp810.c new file +5
-0+default : +return -EINVAL ; + } + +return -EINVAL ;Why is dead code required?
Removed dead code.
Quoted 6 lines
...
Quoted 1 line
+
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.
Quoted 4 lines
Signature · 3 lines
-- With Best Regards, Andy Shevchenko
Regards,
Akhilesh
Re: [PATCH 2/2] iio: pressure: adp810: Add driver for adp810 sensor
Quoted 2 lines · Akhilesh Patil
Quoted 1 line · Andy Shevchenko
On Sat, Oct 11, 2025 at 3:25 PM Akhilesh Patil <[email protected]> wrote:
...
Quoted 24 lines
Quoted 5 lines
Quoted 5 lines
drivers/iio/pressure/adp810.c new file +5
-0+struct adp810_read_buf { +u8 dp_msb; +u8 dp_lsb; +u8 dp_crc; +u8 tmp_msb;Quoted 8 lines
drivers/iio/pressure/adp810.c line 37
Quoted 6 lines
drivers/iio/pressure/adp810.c new file +6
-0+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.__packedwill 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.
Quoted 3 lines
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.)
...
Quoted 8 lines
Quoted 7 lines
drivers/iio/pressure/adp810.c line 43
Quoted 5 lines
drivers/iio/pressure/adp810.c new file +5
-0+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?
Quoted 10 lines
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.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Akhilesh Patil
maintainerRe: [PATCH 2/2] iio: pressure: adp810: Add driver for adp810 sensor
Quoted 30 lines · Andy Shevchenko
Quoted 2 lines · Akhilesh Patil
Quoted 1 line · Andy Shevchenko
On Sat, Oct 11, 2025 at 3:25 PM Akhilesh Patil <[email protected]> wrote:
...
Quoted 24 lines
Quoted 5 lines
Quoted 5 lines
drivers/iio/pressure/adp810.c new file +5
-0+struct adp810_read_buf { +u8 dp_msb; +u8 dp_lsb; +u8 dp_crc; +u8 tmp_msb;Quoted 8 lines
drivers/iio/pressure/adp810.c line 37
Quoted 6 lines
drivers/iio/pressure/adp810.c new file +6
-0+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.__packedwill 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.
Quoted 8 lines
Quoted 3 lines
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
__be16types, the__packedis required for that to be properly placed in the memory.)
Sure. Added comment explaning this in v4.
Regards,
Akhilesh
Quoted 30 lines
...
Quoted 8 lines
Quoted 7 lines
drivers/iio/pressure/adp810.c line 43
Quoted 5 lines
drivers/iio/pressure/adp810.c new file +5
-0+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?
Quoted 10 lines
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.
Signature · 2 lines
With Best Regards, Andy Shevchenko
Jonathan Cameron
maintainerRe: [PATCH 2/2] iio: pressure: adp810: Add driver for adp810 sensor
In reply to Akhilesh PatilOn Sat, 11 Oct 2025 17:55:28 +0530
Akhilesh Patil <[email protected]> wrote:
Hi Akhilesh,
Thanks for sending this and a late welcome to IIO.
Quoted 5 lines
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.
Quoted 3 lines
Tested on TI am62x sk board with sensor connected at i2c-2
Signed-off-by Akhilesh Patil <[email protected]>
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
Quoted 6 lines
drivers/iio/pressure/adp810.c new file +0
-0
drivers/iio/pressure/adp810.c line 9
Quoted 9 lines
drivers/iio/pressure/adp810.c new file +9
-0@@ -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];
drivers/iio/pressure/adp810.c line 32
Quoted 3 lines
drivers/iio/pressure/adp810.c new file +3
-0+u8 dp_crc; +u8 tmp_msb; +u8 tmp_lsb;
__be16_tmp;
drivers/iio/pressure/adp810.c line 35
Quoted 3 lines
drivers/iio/pressure/adp810.c new file +3
-0+u8 tmp_crc; +u8 sf_msb; +u8 sf_lsb;
__be16 sf;
drivers/iio/pressure/adp810.c line 37
Quoted 2 lines
drivers/iio/pressure/adp810.c new file +2
-0+u8 sf_crc; +} __packed;
With packed (which you didn't need previously).
(more below)
drivers/iio/pressure/adp810.c line 48
Quoted 11 lines
drivers/iio/pressure/adp810.c new file +11
-0+ +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.
Quoted 43 lines
drivers/iio/pressure/adp810.c new file +43
-0+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) +{
drivers/iio/pressure/adp810.c line 101
Quoted 10 lines
drivers/iio/pressure/adp810.c new file +10
-0+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.
drivers/iio/pressure/adp810.c line 109
Quoted 8 lines
drivers/iio/pressure/adp810.c new file +8
-0+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.
Quoted 30 lines
drivers/iio/pressure/adp810.c new file +29
-0+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;
drivers/iio/pressure/adp810.c line 166
Quoted 10 lines
drivers/iio/pressure/adp810.c new file +10
-0+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.
drivers/iio/pressure/adp810.c
Quoted 2 lines
drivers/iio/pressure/adp810.c new file +2
-0+ + 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.
drivers/iio/pressure/adp810.c line 178
Quoted 10 lines
drivers/iio/pressure/adp810.c new file +10
-0+ 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.
Quoted 1 line
+}
Akhilesh Patil
maintainerRe: [PATCH 2/2] iio: pressure: adp810: Add driver for adp810 sensor
Quoted 6 lines · Jonathan Cameron
On Sat, 11 Oct 2025 17:55:28 +0530
Akhilesh Patil <[email protected]> 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.
Quoted 7 lines
Quoted 6 lines
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.
Quoted 11 lines
Quoted 4 lines
Tested on TI am62x sk board with sensor connected at i2c-2
Signed-off-by Akhilesh Patil <[email protected]>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.
Quoted 33 lines
Quoted 8 lines
drivers/iio/pressure/adp810.c new file +0
-0drivers/iio/pressure/adp810.c line 9
Quoted 9 lines
drivers/iio/pressure/adp810.c new file +9
-0@@ -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;
__be16dp;
or u8 dp[2];drivers/iio/pressure/adp810.c line 32
Quoted 3 lines
drivers/iio/pressure/adp810.c new file +3
-0+u8 dp_crc; +u8 tmp_msb; +u8 tmp_lsb;
__be16_tmp;drivers/iio/pressure/adp810.c line 35
Quoted 3 lines
drivers/iio/pressure/adp810.c new file +3
-0+u8 tmp_crc; +u8 sf_msb; +u8 sf_lsb;
__be16sf;
Quoted 4 lines
drivers/iio/pressure/adp810.c line 37
Quoted 2 lines
drivers/iio/pressure/adp810.c new file +2
-0+u8 sf_crc; +} __packed;With packed (which you didn't need previously).
(more below)
Yes. Used __be16 to indicate big endian.
Quoted 14 lines
drivers/iio/pressure/adp810.c line 48
Quoted 12 lines
drivers/iio/pressure/adp810.c new file +11
-0+ +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.
Quoted 23 lines
Quoted 23 lines
drivers/iio/pressure/adp810.c new file +23
-0+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);
Quoted 8 lines
drivers/iio/pressure/adp810.c line 101
Quoted 4 lines
drivers/iio/pressure/adp810.c new file +4
-0+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->devhere.
Probably add a local variable struct device *dev, to avoid making thedev_err()lines even longer.
Agree. Will use parent dev in all dev_err() calls.
Quoted 12 lines
drivers/iio/pressure/adp810.c line 109
Quoted 9 lines
drivers/iio/pressure/adp810.c new file +8
-0+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
Quoted 3 lines
*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.
Quoted 34 lines
Quoted 20 lines
drivers/iio/pressure/adp810.c new file +18
-0+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;drivers/iio/pressure/adp810.c line 166
Quoted 10 lines
drivers/iio/pressure/adp810.c new file +10
-0+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().
Quoted 7 lines
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()
Quoted 8 lines
drivers/iio/pressure/adp810.c
Quoted 3 lines
drivers/iio/pressure/adp810.c new file +2
-0+ + 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";
Quoted 12 lines
drivers/iio/pressure/adp810.c line 178
Quoted 10 lines
drivers/iio/pressure/adp810.c new file +10
-0+ 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;
Quoted 1 line
As that clearly indicates to the reader that you can't get here in an error case.
Yes. Fixed.
Regards,
Akhilesh