[PATCH v4 1/3] dt-bindings: vendor-prefixes: add aosong

[PATCH v4 1/3] dt-bindings: vendor-prefixes: add aosong

[Anshul Dalal]

Aosong Electronic Co., LTD. is a supplier for MEMS sensors such as AHT20
temperature and humidity sensor under the brand name Asair

Signed-off-by: Anshul Dalal <anshulusr@gmail.com>
---

Changes for v4:
- no updates

v3: https://lore.kernel.org/lkml/20231121095800.2180870-1-anshulusr@gmail.com/

Changes for v3:
- no updates

v2: https://lore.kernel.org/lkml/20231115125810.1394854-1-anshulusr@gmail.com/

Changes for v2:
- Changed vendor prefix from asair to aosong

v1: https://lore.kernel.org/lkml/20231107173100.62715-1-anshulusr@gmail.com/
---
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 573578db9509..48d4ff635562 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -117,6 +117,8 @@ patternProperties:
     description: Andes Technology Corporation
   "^anvo,.*":
     description: Anvo-Systems Dresden GmbH
+  "^aosong,.*":
+    description: Guangzhou Aosong Electronic Co., Ltd.
   "^apm,.*":
     description: Applied Micro Circuits Corporation (APM)
   "^apple,.*":
-- 
2.42.1

[PATCH v4 2/3] dt-bindings: iio: chemical: add aosong,ags02ma

[Anshul Dalal]

Add bindings for Aosong AGS02MA TVOC sensor.

The sensor communicates over i2c with the default address 0x1a.
TVOC values can be read in the units of ppb and ug/m^3 at register 0x00.

Datasheet:
  https://asairsensors.com/wp-content/uploads/2021/09/AGS02MA.pdf
Product-Page:
  http://www.aosong.com/m/en/products-33.html

Signed-off-by: Anshul Dalal <anshulusr@gmail.com>
---

Changes for v4:
- Changed node name from 'light-sensor' to 'voc-sensor'

v3: https://lore.kernel.org/lkml/20231121095800.2180870-2-anshulusr@gmail.com/

Changes for v3:
- Fixed commit message
- Removed "asair,ags02ma" compatible

v2: https://lore.kernel.org/lkml/20231115125810.1394854-2-anshulusr@gmail.com/

Changes for v2:
- Removed device from trivial-devices
- Added standalone binding with vdd-supply property

v1: https://lore.kernel.org/lkml/20231107173100.62715-2-anshulusr@gmail.com/
---
 .../bindings/iio/chemical/aosong,ags02ma.yaml | 46 +++++++++++++++++++
 1 file changed, 46 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/chemical/aosong,ags02ma.yaml

diff --git a/Documentation/devicetree/bindings/iio/chemical/aosong,ags02ma.yaml b/Documentation/devicetree/bindings/iio/chemical/aosong,ags02ma.yaml
new file mode 100644
index 000000000000..c176a6e102ac
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/chemical/aosong,ags02ma.yaml
@@ -0,0 +1,46 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/chemical/aosong,ags02ma.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Aosong AGS02MA VOC Sensor
+
+description: |
+  AGS02MA is an TVOC (Total Volatile Organic Compounds) i2c sensor with default
+  address of 0x1a.
+
+  Datasheet:
+    https://asairsensors.com/wp-content/uploads/2021/09/AGS02MA.pdf
+
+maintainers:
+  - Anshul Dalal <anshulusr@gmail.com>
+
+properties:
+  compatible:
+    enum:
+      - aosong,ags02ma
+
+  reg:
+    maxItems: 1
+
+  vdd-supply: true
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        voc-sensor@1a {
+            compatible = "aosong,ags02ma";
+            reg = <0x1a>;
+            vdd-supply = <&vdd_regulator>;
+        };
+    };
-- 
2.42.1

Re: [PATCH v4 2/3] dt-bindings: iio: chemical: add aosong,ags02ma

[Krzysztof Kozlowski]

On 25/11/2023 11:01, Anshul Dalal wrote:
> Add bindings for Aosong AGS02MA TVOC sensor.
> 
> The sensor communicates over i2c with the default address 0x1a.
> TVOC values can be read in the units of ppb and ug/m^3 at register 0x00.
> 
> Datasheet:
>   https://asairsensors.com/wp-content/uploads/2021/09/AGS02MA.pdf
> Product-Page:
>   http://www.aosong.com/m/en/products-33.html
> 
> Signed-off-by: Anshul Dalal <anshulusr@gmail.com>


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


---

This is an automated instruction, just in case, because many review tags
are being ignored. If you know the process, you can skip it (please do
not feel offended by me posting it here - no bad intentions intended).
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, under or above your Signed-off-by tag. Tag is "received", when
provided in a message replied to you on the mailing list. Tools like b4
can help here. 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.

https://elixir.bootlin.com/linux/v6.5-rc3/source/Documentation/process/submitting-patches.rst#L577

Best regards,
Krzysztof

Re: [PATCH v4 2/3] dt-bindings: iio: chemical: add aosong,ags02ma

[Jonathan Cameron]

On Sat, 25 Nov 2023 15:31:37 +0530
Anshul Dalal <anshulusr@gmail.com> wrote:

> Add bindings for Aosong AGS02MA TVOC sensor.
> 
> The sensor communicates over i2c with the default address 0x1a.
> TVOC values can be read in the units of ppb and ug/m^3 at register 0x00.
> 
> Datasheet:
>   https://asairsensors.com/wp-content/uploads/2021/09/AGS02MA.pdf
> Product-Page:
>   http://www.aosong.com/m/en/products-33.html
> 
> Signed-off-by: Anshul Dalal <anshulusr@gmail.com>
> ---
> 
> Changes for v4:
> - Changed node name from 'light-sensor' to 'voc-sensor'
> 
> v3: https://lore.kernel.org/lkml/20231121095800.2180870-2-anshulusr@gmail.com/
> 
> Changes for v3:
> - Fixed commit message
> - Removed "asair,ags02ma" compatible
> 
> v2: https://lore.kernel.org/lkml/20231115125810.1394854-2-anshulusr@gmail.com/
> 
> Changes for v2:
> - Removed device from trivial-devices
> - Added standalone binding with vdd-supply property
> 
> v1: https://lore.kernel.org/lkml/20231107173100.62715-2-anshulusr@gmail.com/
> ---
>  .../bindings/iio/chemical/aosong,ags02ma.yaml | 46 +++++++++++++++++++
>  1 file changed, 46 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/chemical/aosong,ags02ma.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/chemical/aosong,ags02ma.yaml b/Documentation/devicetree/bindings/iio/chemical/aosong,ags02ma.yaml
> new file mode 100644
> index 000000000000..c176a6e102ac
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/chemical/aosong,ags02ma.yaml
> @@ -0,0 +1,46 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/chemical/aosong,ags02ma.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Aosong AGS02MA VOC Sensor
> +
> +description: |
> +  AGS02MA is an TVOC (Total Volatile Organic Compounds) i2c sensor with default
> +  address of 0x1a.
> +
> +  Datasheet:
> +    https://asairsensors.com/wp-content/uploads/2021/09/AGS02MA.pdf
> +
> +maintainers:
> +  - Anshul Dalal <anshulusr@gmail.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - aosong,ags02ma
> +
> +  reg:
> +    maxItems: 1
> +
> +  vdd-supply: true

Similar to other review (I put more background there)
Convention these days at least is to always require powersupplies
that the device cannot work with out (even if you can use a fixed
supply and rely on the regulator framework papering over that).
We want to distinguish generally optional supplies from ones we might
not specify in a given DT.

Jonathan

> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        voc-sensor@1a {
> +            compatible = "aosong,ags02ma";
> +            reg = <0x1a>;
> +            vdd-supply = <&vdd_regulator>;
> +        };
> +    };

[PATCH v4 3/3] iio: chemical: add support for Aosong AGS02MA

[Anshul Dalal]

A simple driver for the TVOC (Total Volatile Organic Compounds)
sensor from Aosong: AGS02MA

Steps in reading the VOC sensor value over i2c:
  1. Read 5 bytes from the register `AGS02MA_TVOC_READ_REG` [0x00]
  2. The first 4 bytes are taken as the big endian sensor data with final
     byte being the CRC
  3. The CRC is verified and the value is returned over an
     `IIO_CHAN_INFO_RAW` channel as percents

Tested on Raspberry Pi Zero 2W

Datasheet:
  https://asairsensors.com/wp-content/uploads/2021/09/AGS02MA.pdf
Product-Page:
  http://www.aosong.com/m/en/products-33.html

Signed-off-by: Anshul Dalal <anshulusr@gmail.com>
---

Changes for v4:
- Fixed warning: unused variable 'ags02ma_of_table'
- Fixed warning: unsigned 'ret' is never less than zero in
  `ags02ma_register_read`

v3: https://lore.kernel.org/lkml/20231121095800.2180870-3-anshulusr@gmail.com/

Changes for v3:
- Added of_device_id

v2: https://lore.kernel.org/lkml/20231115125810.1394854-3-anshulusr@gmail.com/

Changes for v2:
- Fixed Kconfig not selecting CRC8 (used to be `select crc8`)
- Changed instances of asair to aosong
- Report raw readings in percents instead of ppb
- Added myself as maintainer for the device binding

v1: https://lore.kernel.org/lkml/20231107173100.62715-3-anshulusr@gmail.com/
---
 MAINTAINERS                    |   7 ++
 drivers/iio/chemical/Kconfig   |  11 +++
 drivers/iio/chemical/Makefile  |   1 +
 drivers/iio/chemical/ags02ma.c | 168 +++++++++++++++++++++++++++++++++
 4 files changed, 187 insertions(+)
 create mode 100644 drivers/iio/chemical/ags02ma.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 81d5fc0bba68..ba3c950aca1b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3028,6 +3028,13 @@ S:	Supported
 W:	http://www.akm.com/
 F:	drivers/iio/magnetometer/ak8974.c
 
+AOSONG AGS02MA TVOC SENSOR DRIVER
+M:	Anshul Dalal <anshulusr@gmail.com>
+L:	linux-iio@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/iio/chemical/aosong,ags02ma.yaml
+F:	drivers/iio/chemical/ags02ma.c
+
 ASC7621 HARDWARE MONITOR DRIVER
 M:	George Joseph <george.joseph@fairview5.com>
 L:	linux-hwmon@vger.kernel.org
diff --git a/drivers/iio/chemical/Kconfig b/drivers/iio/chemical/Kconfig
index c30657e10ee1..02649ab81b3c 100644
--- a/drivers/iio/chemical/Kconfig
+++ b/drivers/iio/chemical/Kconfig
@@ -5,6 +5,17 @@
 
 menu "Chemical Sensors"
 
+config AOSONG_AGS02MA
+	tristate "Aosong AGS02MA TVOC sensor driver"
+	depends on I2C
+	select CRC8
+	help
+	  Say Y here to build support for Aosong AGS02MA TVOC (Total Volatile
+	  Organic Compounds) sensor.
+
+	  To compile this driver as module, choose M here: the module will be
+	  called ags02ma.
+
 config ATLAS_PH_SENSOR
 	tristate "Atlas Scientific OEM SM sensors"
 	depends on I2C
diff --git a/drivers/iio/chemical/Makefile b/drivers/iio/chemical/Makefile
index a11e777a7a00..2f3dee8bb779 100644
--- a/drivers/iio/chemical/Makefile
+++ b/drivers/iio/chemical/Makefile
@@ -4,6 +4,7 @@
 #
 
 # When adding new entries keep the list in alphabetical order
+obj-$(CONFIG_AOSONG_AGS02MA)	+= ags02ma.o
 obj-$(CONFIG_ATLAS_PH_SENSOR)	+= atlas-sensor.o
 obj-$(CONFIG_ATLAS_EZO_SENSOR)	+= atlas-ezo-sensor.o
 obj-$(CONFIG_BME680) += bme680_core.o
diff --git a/drivers/iio/chemical/ags02ma.c b/drivers/iio/chemical/ags02ma.c
new file mode 100644
index 000000000000..b23160eac99f
--- /dev/null
+++ b/drivers/iio/chemical/ags02ma.c
@@ -0,0 +1,168 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (C) 2023 Anshul Dalal <anshulusr@gmail.com>
+ *
+ * Driver for Aosong AGS02MA
+ *
+ * Datasheet:
+ *   https://asairsensors.com/wp-content/uploads/2021/09/AGS02MA.pdf
+ * Product Page:
+ *   http://www.aosong.com/m/en/products-33.html
+ *
+ * TODO:
+ *   - Support for ug/m^3 units of measurement
+ */
+
+#include <linux/crc8.h>
+#include <linux/delay.h>
+#include <linux/i2c.h>
+#include <linux/iio/iio.h>
+#include <linux/module.h>
+
+#define AGS02MA_DEVICE_NAME		   "ags02ma"
+
+#define AGS02MA_TVOC_READ_REG		   0x00
+#define AGS02MA_VERSION_REG		   0x11
+
+#define AGS02MA_VERSION_PROCESSING_DELAY   30
+#define AGS02MA_TVOC_READ_PROCESSING_DELAY 1500
+
+#define AGS02MA_CRC8_INIT		   0xff
+#define AGS02MA_CRC8_POLYNOMIAL		   0x31
+#define AGS02MA_PPB_PERCENT_CONVERSION     10000000
+
+DECLARE_CRC8_TABLE(ags02ma_crc8_table);
+
+struct ags02ma_data {
+	struct i2c_client *client;
+};
+
+struct ags02ma_reading {
+	__be32 data;
+	u8 crc;
+} __packed;
+
+static u32 ags02ma_register_read(struct i2c_client *client, u8 reg, u16 delay)
+{
+	int ret;
+	u8 crc;
+	struct ags02ma_reading read_buffer;
+
+	ret = i2c_master_send(client, &reg, sizeof(reg));
+	if (ret < 0) {
+		dev_err(&client->dev,
+			"Failed to send data to register 0x%x: %d", reg, ret);
+		return ret;
+	}
+
+	/* Processing Delay, Check Table 7.7 in the datasheet */
+	msleep_interruptible(delay);
+
+	ret = i2c_master_recv(client, (u8 *)&read_buffer, sizeof(read_buffer));
+	if (ret < 0) {
+		dev_err(&client->dev,
+			"Failed to receive from register 0x%x: %d", reg, ret);
+		return ret;
+	}
+
+	crc = crc8(ags02ma_crc8_table, (u8 *)&read_buffer.data,
+		   sizeof(read_buffer.data), AGS02MA_CRC8_INIT);
+	if (crc != read_buffer.crc) {
+		dev_err(&client->dev, "CRC error\n");
+		return -EIO;
+	}
+
+	return be32_to_cpu(read_buffer.data);
+}
+
+static int ags02ma_read_raw(struct iio_dev *iio_device,
+			    struct iio_chan_spec const *chan, int *val,
+			    int *val2, long mask)
+{
+	int ret;
+	struct ags02ma_data *data = iio_priv(iio_device);
+
+	if (mask == IIO_CHAN_INFO_RAW) {
+		/* The sensor reads data as ppb */
+		ret = ags02ma_register_read(data->client, AGS02MA_TVOC_READ_REG,
+					    AGS02MA_TVOC_READ_PROCESSING_DELAY);
+		if (ret < 0)
+			return ret;
+		*val = ret;
+		*val2 = AGS02MA_PPB_PERCENT_CONVERSION;
+		return IIO_VAL_FRACTIONAL;
+	} else {
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info ags02ma_info = {
+	.read_raw = ags02ma_read_raw,
+};
+
+static const struct iio_chan_spec ags02ma_channels[] = {
+	{ .type = IIO_CONCENTRATION,
+	  .channel2 = IIO_MOD_VOC,
+	  .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) }
+};
+
+static int ags02ma_probe(struct i2c_client *client)
+{
+	int ret;
+	struct ags02ma_data *data;
+	struct iio_dev *indio_dev;
+	u32 version;
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	crc8_populate_msb(ags02ma_crc8_table, AGS02MA_CRC8_POLYNOMIAL);
+
+	ret = ags02ma_register_read(client, AGS02MA_VERSION_REG,
+				    AGS02MA_VERSION_PROCESSING_DELAY);
+	if (ret < 0) {
+		dev_err(&client->dev, "Failed to read device version: %d", ret);
+		return ret;
+	}
+	version = ret;
+	dev_dbg(&client->dev, "Aosong AGS02MA, Version: 0x%x", version);
+
+	data = iio_priv(indio_dev);
+	i2c_set_clientdata(client, indio_dev);
+	data->client = client;
+	indio_dev->info = &ags02ma_info;
+	indio_dev->channels = ags02ma_channels;
+	indio_dev->num_channels = ARRAY_SIZE(ags02ma_channels);
+	indio_dev->name = AGS02MA_DEVICE_NAME;
+
+	return devm_iio_device_register(&client->dev, indio_dev);
+}
+
+static const struct i2c_device_id ags02ma_id_table[] = {
+	{ AGS02MA_DEVICE_NAME, 0 },
+	{ /* Sentinel */ }
+};
+MODULE_DEVICE_TABLE(i2c, ags02ma_id_table);
+
+#ifdef CONFIG_OF
+static const struct of_device_id ags02ma_of_table[] = {
+	{ .compatible = "aosong,ags02ma"},
+	{ /* Sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, ags02ma_of_table);
+#endif
+
+static struct i2c_driver ags02ma_driver = {
+	.driver = {
+		.name = AGS02MA_DEVICE_NAME,
+		.of_match_table = of_match_ptr(ags02ma_of_table),
+	},
+	.id_table = ags02ma_id_table,
+	.probe = ags02ma_probe,
+};
+module_i2c_driver(ags02ma_driver);
+
+MODULE_AUTHOR("Anshul Dalal <anshulusr@gmail.com>");
+MODULE_DESCRIPTION("Aosong AGS02MA TVOC Driver");
+MODULE_LICENSE("GPL");
-- 
2.42.1

Re: [PATCH v4 3/3] iio: chemical: add support for Aosong AGS02MA

[Jonathan Cameron]

On Sat, 25 Nov 2023 15:31:38 +0530
Anshul Dalal <anshulusr@gmail.com> wrote:
Hi Anshul

Comments inline.


> A simple driver for the TVOC (Total Volatile Organic Compounds)
> sensor from Aosong: AGS02MA
> 
> Steps in reading the VOC sensor value over i2c:
>   1. Read 5 bytes from the register `AGS02MA_TVOC_READ_REG` [0x00]
>   2. The first 4 bytes are taken as the big endian sensor data with final
>      byte being the CRC
>   3. The CRC is verified and the value is returned over an
>      `IIO_CHAN_INFO_RAW` channel as percents

We have standard units for VOC sensors of percents. So if your device
already outputs in that you should make it an IIO_CHAN_INFO_PROCESSED
to reflect that.  If not, I'd expect to see a IIO_CHAN_INFO_SCALE
for any necessary conversion.

> 
> Tested on Raspberry Pi Zero 2W
> 
> Datasheet:
>   https://asairsensors.com/wp-content/uploads/2021/09/AGS02MA.pdf
Make this a formal tag in the tag block below.
> Product-Page:
>   http://www.aosong.com/m/en/products-33.html
This one isn't a standard tag, so fine to keep as you have it here.
> 
> Signed-off-by: Anshul Dalal <anshulusr@gmail.com>

...

> diff --git a/drivers/iio/chemical/ags02ma.c b/drivers/iio/chemical/ags02ma.c
> new file mode 100644
> index 000000000000..b23160eac99f
> --- /dev/null
> +++ b/drivers/iio/chemical/ags02ma.c
> @@ -0,0 +1,168 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2023 Anshul Dalal <anshulusr@gmail.com>
> + *
> + * Driver for Aosong AGS02MA
> + *
> + * Datasheet:
> + *   https://asairsensors.com/wp-content/uploads/2021/09/AGS02MA.pdf
> + * Product Page:
> + *   http://www.aosong.com/m/en/products-33.html
> + *
> + * TODO:
> + *   - Support for ug/m^3 units of measurement

Why? I'd assume that's some linear conversion of the percent value.  Leave
that to userspace.

> + */
> +
> +#include <linux/crc8.h>
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/iio/iio.h>
> +#include <linux/module.h>
> +
> +#define AGS02MA_DEVICE_NAME		   "ags02ma"
> +
> +#define AGS02MA_TVOC_READ_REG		   0x00
> +#define AGS02MA_VERSION_REG		   0x11
> +
> +#define AGS02MA_VERSION_PROCESSING_DELAY   30
> +#define AGS02MA_TVOC_READ_PROCESSING_DELAY 1500
> +
> +#define AGS02MA_CRC8_INIT		   0xff
> +#define AGS02MA_CRC8_POLYNOMIAL		   0x31
> +#define AGS02MA_PPB_PERCENT_CONVERSION     10000000
> +
> +DECLARE_CRC8_TABLE(ags02ma_crc8_table);
> +
> +struct ags02ma_data {
> +	struct i2c_client *client;
> +};
> +
> +struct ags02ma_reading {
> +	__be32 data;
> +	u8 crc;
> +} __packed;
> +
> +static u32 ags02ma_register_read(struct i2c_client *client, u8 reg, u16 delay)
> +{
> +	int ret;
> +	u8 crc;
> +	struct ags02ma_reading read_buffer;
> +
> +	ret = i2c_master_send(client, &reg, sizeof(reg));
> +	if (ret < 0) {
> +		dev_err(&client->dev,
> +			"Failed to send data to register 0x%x: %d", reg, ret);
> +		return ret;
> +	}
> +
> +	/* Processing Delay, Check Table 7.7 in the datasheet */
> +	msleep_interruptible(delay);
> +
> +	ret = i2c_master_recv(client, (u8 *)&read_buffer, sizeof(read_buffer));
> +	if (ret < 0) {
> +		dev_err(&client->dev,
> +			"Failed to receive from register 0x%x: %d", reg, ret);
> +		return ret;

function returns a u32... 
I'd separate the return value from the data read.  Use
a u32 *val parameter for the data.

> +	}
> +
> +	crc = crc8(ags02ma_crc8_table, (u8 *)&read_buffer.data,
> +		   sizeof(read_buffer.data), AGS02MA_CRC8_INIT);
> +	if (crc != read_buffer.crc) {
> +		dev_err(&client->dev, "CRC error\n");
> +		return -EIO;
> +	}
> +
> +	return be32_to_cpu(read_buffer.data);

> +}
> +
> +static int ags02ma_read_raw(struct iio_dev *iio_device,
> +			    struct iio_chan_spec const *chan, int *val,
> +			    int *val2, long mask)
> +{
> +	int ret;
> +	struct ags02ma_data *data = iio_priv(iio_device);
> +
> +	if (mask == IIO_CHAN_INFO_RAW) {
> +		/* The sensor reads data as ppb */
> +		ret = ags02ma_register_read(data->client, AGS02MA_TVOC_READ_REG,
> +					    AGS02MA_TVOC_READ_PROCESSING_DELAY);
> +		if (ret < 0)
> +			return ret;
> +		*val = ret;
> +		*val2 = AGS02MA_PPB_PERCENT_CONVERSION;

Use IIO_CHAN_INFO_SCALE to deal with the division. Not all applications will care, so make it
a userspace decision rather than doing it in driver (or a consumer decision if for some
reason we end up with an inkernel consumer of VOC data)  If you enhance this driver
later to support buffering, you will have made it very challenging by doing
this conversion in driver.

> +		return IIO_VAL_FRACTIONAL;
> +	} else {
> +		return -EINVAL;
	Flip logic to exit in error case out of main line of code flow.

	if (mask != IIO_CHAN_INFO_RAW)
		return -EINVAL;
	
	/* The sensor read as ppb */
	ret = ...

> +	}
> +}
> +
> +static const struct iio_info ags02ma_info = {
> +	.read_raw = ags02ma_read_raw,
> +};
> +
> +static const struct iio_chan_spec ags02ma_channels[] = {
> +	{ .type = IIO_CONCENTRATION,
	{
		.type = ..
or given there is one, flatten it a level and just fill in
1 explicitly for num channels.

> +	  .channel2 = IIO_MOD_VOC,
> +	  .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) }
						  RAW),
	},

> +};
> +
> +static int ags02ma_probe(struct i2c_client *client)
> +{
> +	int ret;
> +	struct ags02ma_data *data;
> +	struct iio_dev *indio_dev;
> +	u32 version;
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	crc8_populate_msb(ags02ma_crc8_table, AGS02MA_CRC8_POLYNOMIAL);
> +
> +	ret = ags02ma_register_read(client, AGS02MA_VERSION_REG,
> +				    AGS02MA_VERSION_PROCESSING_DELAY);
> +	if (ret < 0) {
> +		dev_err(&client->dev, "Failed to read device version: %d", ret);
> +		return ret;

Prefer
	return dev_err_probe(&client->dev, ret, "Failed to read device version\n");
for any errors that show up in probe.
It's shorter + provides advantages if you end up adding some calls that
can defer probing later.

> +	}
> +	version = ret;
> +	dev_dbg(&client->dev, "Aosong AGS02MA, Version: 0x%x", version);
> +
> +	data = iio_priv(indio_dev);
> +	i2c_set_clientdata(client, indio_dev);

Why? I don't think you use it (which is expected in a simple driver - tend
to need this only when adding power management or having to do explicit
remove() handling for some reason.

> +	data->client = client;
> +	indio_dev->info = &ags02ma_info;
> +	indio_dev->channels = ags02ma_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(ags02ma_channels);
> +	indio_dev->name = AGS02MA_DEVICE_NAME;

I'm not a fan of defines for name strings because I'd rather just see them
where they are used - why hid this away?  It's not saving code
and a string is as good as a define in all the places it's used
(there is not real reason they have to have the same value, even though
they happen to do so in this driver).

> +
> +	return devm_iio_device_register(&client->dev, indio_dev);
> +}
> +
> +static const struct i2c_device_id ags02ma_id_table[] = {
> +	{ AGS02MA_DEVICE_NAME, 0 },

Don't put the 0 as it implies some data in there, when it's never
read (C will put a 0 in there anyway but that doesn't matter)

> +	{ /* Sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(i2c, ags02ma_id_table);
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id ags02ma_of_table[] = {
> +	{ .compatible = "aosong,ags02ma"},
> +	{ /* Sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, ags02ma_of_table);
> +#endif

With of_match_ptr() dropped below y ou will again not need these.

> +
> +static struct i2c_driver ags02ma_driver = {
> +	.driver = {
> +		.name = AGS02MA_DEVICE_NAME,
> +		.of_match_table = of_match_ptr(ags02ma_of_table),
Never have of_match_ptr() in an IIO driver.
It breaks ACPI PRP0001 based bindings which are frequently used with
IIO devices - also saves trivial amoutn of space.

> +	},

All minor stuff and great to have more of these VOC drivers in supported.

Thanks,

Jonathan

Re: [PATCH v4 1/3] dt-bindings: vendor-prefixes: add aosong

[Krzysztof Kozlowski]

On 25/11/2023 11:01, Anshul Dalal wrote:
> Aosong Electronic Co., LTD. is a supplier for MEMS sensors such as AHT20
> temperature and humidity sensor under the brand name Asair
> 
> Signed-off-by: Anshul Dalal <anshulusr@gmail.com>
> ---
> 

This is a friendly reminder during the review process.

It looks like you received a tag and forgot to add it.

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, under or above your Signed-off-by tag. Tag is "received", when
provided in a message replied to you on the mailing list. Tools like b4
can help here. 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.

https://elixir.bootlin.com/linux/v6.5-rc3/source/Documentation/process/submitting-patches.rst#L577

If a tag was not added on purpose, please state why and what changed.

Best regards,
Krzysztof