Re: [PATCH v2] hwmon: (aht10) Add support for compatible aht20

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, May 24, 2023 at 08:46:11PM +0200, Kirill Yatsenko wrote:
> Add support for compatible AHT20 temperature/humidity sensor. The only
> difference between the two is that AHT20 has additional crc8 byte.
> 
> It seems like AHT15 is also supported by the driver but it wasn't
> verified and tested yet.
> 
> Tested on Beaglebone black rev C.
> 
> Signed-off-by: Kirill Yatsenko <kiriyatsenko@xxxxxxxxx>
> ---
> v2: Add aht20 to aht10_id instead of using Kconfig flag, remove ifdefs
>     as suggested by Guenter Roeck <linux@xxxxxxxxxxxx>
> 
>  Documentation/hwmon/aht10.rst | 20 ++++++---
>  drivers/hwmon/Kconfig         |  5 ++-
>  drivers/hwmon/aht10.c         | 81 +++++++++++++++++++++++++++--------
>  3 files changed, 80 insertions(+), 26 deletions(-)
> 
> diff --git a/Documentation/hwmon/aht10.rst b/Documentation/hwmon/aht10.rst
> index 4e198c5eb683..213644b4ecba 100644
> --- a/Documentation/hwmon/aht10.rst
> +++ b/Documentation/hwmon/aht10.rst
> @@ -5,32 +5,42 @@ Kernel driver aht10
>  
>  Supported chips:
>  
> -  * Aosong AHT10
> +  * Aosong AHT10/AHT20
>  
>      Prefix: 'aht10'
>  
>      Addresses scanned: None
>  
> -    Datasheet:
> +    Datasheet(AHT10):
>  
>        Chinese: http://www.aosong.com/userfiles/files/media/AHT10%E4%BA%A7%E5%93%81%E6%89%8B%E5%86%8C%20A3%2020201210.pdf
>        English: https://server4.eca.ir/eshop/AHT10/Aosong_AHT10_en_draft_0c.pdf
>  
> +    Datasheet(AHT20):
> +
> +      English: http://www.aosong.com/userfiles/files/media/Data%20Sheet%20AHT20.pdf
> +
>  Author: Johannes Cornelis Draaijer <jcdra1@xxxxxxxxx>
>  
>  
>  Description
>  -----------
>  
> -The AHT10 is a Temperature and Humidity sensor
> +The AHT10/AHT20 is a Temperature and Humidity sensor
>  
>  The address of this i2c device may only be 0x38
>  
> +Special Features
> +----------------
> +
> +AHT20 has additional CRC8 support which is sent as the last byte of the sensor
> +values.
> +
>  Usage Notes
>  -----------
>  
> -This driver does not probe for AHT10 devices, as there is no reliable
> -way to determine if an i2c chip is or isn't an AHT10. The device has
> +This driver does not probe for AHT10/ATH20 devices, as there is no reliable
> +way to determine if an i2c chip is or isn't an AHT10/AHT20. The device has
>  to be instantiated explicitly with the address 0x38. See
>  Documentation/i2c/instantiating-devices.rst for details.
>  
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index fc640201a2de..bf73934a6eee 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -255,10 +255,11 @@ config SENSORS_ADT7475
>  	  will be called adt7475.
>  
>  config SENSORS_AHT10
> -	tristate "Aosong AHT10"
> +	tristate "Aosong AHT10, AHT20"
>  	depends on I2C
> +	select CRC8
>  	help
> -	  If you say yes here, you get support for the Aosong AHT10
> +	  If you say yes here, you get support for the Aosong AHT10 and AHT20
>  	  temperature and humidity sensors
>  
>  	  This driver can also be built as a module. If so, the module
> diff --git a/drivers/hwmon/aht10.c b/drivers/hwmon/aht10.c
> index ec7459575c58..02396ebe6ef5 100644
> --- a/drivers/hwmon/aht10.c
> +++ b/drivers/hwmon/aht10.c
> @@ -1,7 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  
>  /*
> - * aht10.c - Linux hwmon driver for AHT10 Temperature and Humidity sensor
> + * aht10.c - Linux hwmon driver for AHT10/AHT20 Temperature and Humidity sensors
>   * Copyright (C) 2020 Johannes Cornelis Draaijer
>   */
>  
> @@ -10,9 +10,13 @@
>  #include <linux/i2c.h>
>  #include <linux/ktime.h>
>  #include <linux/module.h>
> +#include <linux/crc8.h>
>  
>  #define AHT10_MEAS_SIZE		6
>  
> +#define AHT20_MEAS_SIZE		7
> +#define AHT20_CRC8_POLY		0x31
> +
>  /*
>   * Poll intervals (in milliseconds)
>   */
> @@ -44,9 +48,18 @@
>  
>  #define AHT10_MAX_POLL_INTERVAL_LEN	30
>  
> +enum aht10_variant { aht10, aht20 };
> +
> +static const struct i2c_device_id aht10_id[] = {
> +	{ "aht10", aht10 },
> +	{ "aht20", aht20 },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(i2c, aht10_id);
> +
>  /**
> - *   struct aht10_data - All the data required to operate an AHT10 chip
> - *   @client: the i2c client associated with the AHT10
> + *   struct aht10_data - All the data required to operate an AHT10/AHT20 chip
> + *   @client: the i2c client associated with the AHT10/AHT20
>   *   @lock: a mutex that is used to prevent parallel access to the
>   *          i2c client
>   *   @min_poll_interval: the minimum poll interval
> @@ -56,12 +69,14 @@
>   *                   the chip from warming up due to the heat it generates.
>   *                   If it's unwanted, it can be ignored setting it to
>   *                   it to 0. Default value is 2000 ms
> - *   @previous_poll_time: the previous time that the AHT10
> + *   @previous_poll_time: the previous time that the AHT10/AHT20
>   *                        was polled
>   *   @temperature: the latest temperature value received from
> - *                 the AHT10
> + *                 the AHT10/AHT20
>   *   @humidity: the latest humidity value received from the
> - *              AHT10
> + *              AHT10/AHT20
> + *   @crc8: crc8 support flag
> + *   @meas_size: measurements data size
>   */
>  
>  struct aht10_data {
> @@ -75,11 +90,13 @@ struct aht10_data {
>  	ktime_t previous_poll_time;
>  	int temperature;
>  	int humidity;
> +	bool crc8;
> +	unsigned int meas_size;
>  };
>  
>  /**
> - * aht10_init() - Initialize an AHT10 chip
> - * @data: the data associated with this AHT10 chip
> + * aht10_init() - Initialize an AHT10/AHT20 chip
> + * @data: the data associated with this AHT10/AHT20 chip
>   * Return: 0 if successful, 1 if not
>   */
>  static int aht10_init(struct aht10_data *data)
> @@ -121,8 +138,23 @@ static int aht10_polltime_expired(struct aht10_data *data)
>  	return ktime_after(difference, data->min_poll_interval);
>  }
>  
> +DECLARE_CRC8_TABLE(crc8_table);
> +
> +/**
> + * crc8_check() - check crc of the sensor's measurements
> + * @raw_data: data frame received from sensor(including crc as the last byte)
> + * @count: size of the data frame
> + * Return: 0 if successful, 1 if not
> + */
> +static int crc8_check(u8 *raw_data, int count)
> +{
> +	// crc calculated on the whole frame(including crc byte) should yield
> +	// zero in case of correctly received bytes

Please use standard multi-line comments.

> +	return crc8(crc8_table, raw_data, count, CRC8_INIT_VALUE);
> +}
> +
>  /**
> - * aht10_read_values() - read and parse the raw data from the AHT10
> + * aht10_read_values() - read and parse the raw data from the AHT10/AHT20
>   * @data: the struct aht10_data to use for the lock
>   * Return: 0 if successful, 1 if not
>   */
> @@ -131,7 +163,7 @@ static int aht10_read_values(struct aht10_data *data)
>  	const u8 cmd_meas[] = {AHT10_CMD_MEAS, 0x33, 0x00};
>  	u32 temp, hum;
>  	int res;
> -	u8 raw_data[AHT10_MEAS_SIZE];
> +	u8 raw_data[AHT20_MEAS_SIZE];
>  	struct i2c_client *client = data->client;
>  
>  	mutex_lock(&data->lock);
> @@ -148,14 +180,19 @@ static int aht10_read_values(struct aht10_data *data)
>  
>  	usleep_range(AHT10_MEAS_DELAY, AHT10_MEAS_DELAY + AHT10_DELAY_EXTRA);
>  
> -	res = i2c_master_recv(client, raw_data, AHT10_MEAS_SIZE);
> -	if (res != AHT10_MEAS_SIZE) {
> +	res = i2c_master_recv(client, raw_data, data->meas_size);
> +	if (res != data->meas_size) {
>  		mutex_unlock(&data->lock);
>  		if (res >= 0)
>  			return -ENODATA;
>  		return res;
>  	}
>  
> +	if (data->crc8 && crc8_check(raw_data, data->meas_size)) {
> +		mutex_unlock(&data->lock);
> +		return -ENODATA;

-ENODATA for checksum errors is not appropriate. I would suggest
to use -EIO.

> +	}
> +
>  	hum =   ((u32)raw_data[1] << 12u) |
>  		((u32)raw_data[2] << 4u) |
>  		((raw_data[3] & 0xF0u) >> 4u);
> @@ -292,6 +329,8 @@ static const struct hwmon_chip_info aht10_chip_info = {
>  
>  static int aht10_probe(struct i2c_client *client)
>  {
> +	const struct i2c_device_id *id = i2c_match_id(aht10_id, client);
> +	enum aht10_variant variant = (enum aht10_variant)id->driver_data;

This should not require a type cast.

>  	struct device *device = &client->dev;
>  	struct device *hwmon_dev;
>  	struct aht10_data *data;
> @@ -307,6 +346,16 @@ static int aht10_probe(struct i2c_client *client)
>  	data->min_poll_interval = ms_to_ktime(AHT10_DEFAULT_MIN_POLL_INTERVAL);
>  	data->client = client;
>  
> +	switch (variant) {
> +	case aht20:
> +		data->meas_size = AHT20_MEAS_SIZE;
> +		data->crc8 = true;
> +		crc8_populate_msb(crc8_table, AHT20_CRC8_POLY);
> +		break;
> +	default:
> +		data->meas_size = AHT10_MEAS_SIZE;

Add
		break;

> +	}
> +
>  	mutex_init(&data->lock);
>  
>  	res = aht10_init(data);
> @@ -326,12 +375,6 @@ static int aht10_probe(struct i2c_client *client)
>  	return PTR_ERR_OR_ZERO(hwmon_dev);
>  }
>  
> -static const struct i2c_device_id aht10_id[] = {
> -	{ "aht10", 0 },
> -	{ },
> -};
> -MODULE_DEVICE_TABLE(i2c, aht10_id);
> -
>  static struct i2c_driver aht10_driver = {
>  	.driver = {
>  		.name = "aht10",
> @@ -343,6 +386,6 @@ static struct i2c_driver aht10_driver = {
>  module_i2c_driver(aht10_driver);
>  
>  MODULE_AUTHOR("Johannes Cornelis Draaijer <jcdra1@xxxxxxxxx>");
> -MODULE_DESCRIPTION("AHT10 Temperature and Humidity sensor driver");
> +MODULE_DESCRIPTION("AHT10/AHT20 Temperature and Humidity sensor driver");
>  MODULE_VERSION("1.0");
>  MODULE_LICENSE("GPL v2");
> -- 
> 2.25.1
> 



[Index of Archives]     [LM Sensors]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux