dt-bindings: vendor-prefixes: add aosong
Subsystems (3)
Anshul Dalal
maintainerPATCH v4 1/3 dt-bindings: vendor-prefixes: add aosong
Aosong Electronic Co., LTD. is a supplier for MEMS sensors such as AHT20 temperature and humidity sensor under the brand name Asair
---
Changes for v4:
- no updates
v3: https://lore.kernel.org/lkml/[email protected]/
Changes for v3:
- no updates
v2: https://lore.kernel.org/lkml/[email protected]/
Changes for v2:
- Changed vendor prefix from asair to aosong
v1: https://lore.kernel.org/lkml/[email protected]/
--- Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++ 1 file changed, 2 insertions(+)
Documentation/devicetree/bindings/vendor-prefixes.yaml +2 -0
@@ -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,.*":
Signature · 1 line
2.42.1
Anshul Dalal
maintainerPATCH v4 2/3 dt-bindings: iio: chemical: add aosong,ags02ma
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
---
Changes for v4:
- Changed node name from 'light-sensor' to 'voc-sensor'
v3: https://lore.kernel.org/lkml/[email protected]/
Changes for v3:
- Fixed commit message
- Removed "asair,ags02ma" compatible
v2: https://lore.kernel.org/lkml/[email protected]/
Changes for v2:
- Removed device from trivial-devices
- Added standalone binding with vdd-supply property
v1: https://lore.kernel.org/lkml/[email protected]/
--- .../bindings/iio/chemical/aosong,ags02ma.yaml | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/chemical/aosong,ags02ma.yaml
Documentation/devicetree/bindings/iio/chemical/aosong,ags02ma.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/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 <[email protected]> + +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>; + }; + };
Signature · 1 line
2.42.1
Re: [PATCH v4 2/3] dt-bindings: iio: chemical: add aosong,ags02ma
Quoted 11 lines · 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 register0x00.Datasheet:
https://asairsensors.com/wp-content/uploads/2021/09/AGS02MA.pdf
Product-Page:
http://www.aosong.com/m/en/products-33.htmlSigned-off-by Anshul Dalal <[email protected]>
---
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
Jonathan Cameron
maintainerRe: [PATCH v4 2/3] dt-bindings: iio: chemical: add aosong,ags02ma
In reply to Anshul DalalQuoted 57 lines · On Sat, 25 Nov 2023 15:31:37 +0530 Anshul Dalal <[email protected]> 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 register0x00.Datasheet:
https://asairsensors.com/wp-content/uploads/2021/09/AGS02MA.pdf
Product-Page:
http://www.aosong.com/m/en/products-33.htmlSigned-off-by Anshul Dalal <[email protected]>---
Changes for v4:
- Changed node name from 'light-sensor' to 'voc-sensor'v3: https://lore.kernel.org/lkml/[email protected]/
Changes for v3:
- Fixed commit message
- Removed "asair,ags02ma" compatiblev2: https://lore.kernel.org/lkml/[email protected]/
Changes for v2:
- Removed device from trivial-devices
- Added standalone binding with vdd-supply propertyv1: https://lore.kernel.org/lkml/[email protected]/
--- .../bindings/iio/chemical/aosong,ags02ma.yaml | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/chemical/aosong,ags02ma.yamlDocumentation/devicetree/bindings/iio/chemical/aosong,ags02ma.yaml new file +17
-0@@ -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 <[email protected]>
Documentation/devicetree/bindings/iio/chemical/aosong,ags02ma.yaml line 27
Quoted 10 lines
Documentation/devicetree/bindings/iio/chemical/aosong,ags02ma.yaml new file +10
-0+ +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
Quoted 19 lines
Documentation/devicetree/bindings/iio/chemical/aosong,ags02ma.yaml new file +19
-0+ +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>; + }; + };
Anshul Dalal
maintainerPATCH v4 3/3 iio: chemical: add support for Aosong AGS02MA
In reply to Anshul DalalA 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
---
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/[email protected]/
Changes for v3:
- Added of_device_id
v2: https://lore.kernel.org/lkml/[email protected]/
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/[email protected]/
--- 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
MAINTAINERS +7 -0
@@ -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 <[email protected]> +L: [email protected] +S: Maintained +F: Documentation/devicetree/bindings/iio/chemical/aosong,ags02ma.yaml +F: drivers/iio/chemical/ags02ma.c + ASC7621 HARDWARE MONITOR DRIVER M: George Joseph <[email protected]> L: [email protected]
drivers/iio/chemical/Kconfig +11 -0
@@ -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
drivers/iio/chemical/Makefile +1 -0
@@ -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
drivers/iio/chemical/ags02ma.c new file +168 -0
168 lines not on this page · raw message
Signature · 1 line
2.42.1
Jonathan Cameron
maintainerRe: [PATCH v4 3/3] iio: chemical: add support for Aosong AGS02MA
On Sat, 25 Nov 2023 15:31:38 +0530
Anshul Dalal <[email protected]> wrote:
Hi Anshul
Comments inline.
Quoted 9 lines
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 registerAGS02MA_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 anIIO_CHAN_INFO_RAWchannel 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.
Quoted 5 lines
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.
Quoted 2 lines
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.
Quoted 2 lines
Signed-off-by Anshul Dalal <[email protected]>
...
Quoted 9 lines
drivers/iio/chemical/ags02ma.c new file +3
-0@@ -0,0 +1,168 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* +* Copyright (C) 2023 Anshul Dalal <[email protected]>
drivers/iio/chemical/ags02ma.c line 13
Quoted 10 lines
drivers/iio/chemical/ags02ma.c new file +10
-0+* +* 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.
Quoted 42 lines
drivers/iio/chemical/ags02ma.c new file +42
-0+*/ + +#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, ®,sizeof (reg)); +if (ret <0 ) { +dev_err (&client->dev, +"Failed to send data to register 0x%x: %d" , reg, ret); +return ret;
drivers/iio/chemical/ags02ma.c line 65
Quoted 10 lines
drivers/iio/chemical/ags02ma.c new file +10
-0+ } + +/* 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.
Quoted 18 lines
drivers/iio/chemical/ags02ma.c new file +17
-0+ } + + 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;
drivers/iio/chemical/ags02ma.c line 92
Quoted 10 lines
drivers/iio/chemical/ags02ma.c new file +10
-0+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.
drivers/iio/chemical/ags02ma.c line 95
Quoted 3 lines
drivers/iio/chemical/ags02ma.c new file +3
-0+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 = ...
drivers/iio/chemical/ags02ma.c line 104
Quoted 9 lines
drivers/iio/chemical/ags02ma.c new file +9
-0+ } +} + +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.
drivers/iio/chemical/ags02ma.c line 106
Quoted 2 lines
drivers/iio/chemical/ags02ma.c new file +2
-0+ .channel2 = IIO_MOD_VOC, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) }
RAW), },
Quoted 10 lines
drivers/iio/chemical/ags02ma.c new file +10
-0+}; + +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));
drivers/iio/chemical/ags02ma.c line 126
Quoted 10 lines
drivers/iio/chemical/ags02ma.c new file +10
-0+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.
drivers/iio/chemical/ags02ma.c line 132
Quoted 6 lines
drivers/iio/chemical/ags02ma.c new file +6
-0+ } + 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.
drivers/iio/chemical/ags02ma.c line 137
Quoted 5 lines
drivers/iio/chemical/ags02ma.c new file +5
-0+ 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).
drivers/iio/chemical/ags02ma.c line 143
Quoted 6 lines
drivers/iio/chemical/ags02ma.c new file +6
-0+ +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)
drivers/iio/chemical/ags02ma.c line 154
Quoted 11 lines
drivers/iio/chemical/ags02ma.c new file +11
-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
With of_match_ptr() dropped below y ou will again not need these.
drivers/iio/chemical/ags02ma.c line 159
Quoted 5 lines
drivers/iio/chemical/ags02ma.c new file +5
-0+ +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.
drivers/iio/chemical/ags02ma.c line 160
Quoted 1 line
drivers/iio/chemical/ags02ma.c new file +1
-0+ },
All minor stuff and great to have more of these VOC drivers in supported.
Thanks,
Jonathan
Quoted 6 lines · 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 <[email protected]>---
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