On 12/13/20 1:48 PM, datdenkikniet wrote:
> This patch adds a hwmon driver for the AHT10 Temperature
> and Humidity sensor. It has a maxiumum sample rate, as
> the datasheet states that the chip may heat up if it is
> sampled too often.
>
> Has been tested to work on a raspberrypi0w
>
> Signed-off-by: Johannes Cornelis Draaijer (datdenkikniet) <jcdra1@xxxxxxxxx>
This patch didn't make it to patchwork. On top of that, my e-mail provider
tagged it as spam, based on the following information.
1.0 TVD_RCVD_IP Message was received from an IP address
0.4 NO_DNS_FOR_FROM DNS: Envelope sender has no MX or A DNS records
0.0 SPF_NONE SPF: sender does not publish an SPF Record
0.2 HEADER_FROM_DIFFERENT_DOMAINS From and EnvelopeFrom 2nd level mail domains are different
1.0 FORGED_GMAIL_RCVD 'From' gmail.com does not match 'Received' headers
0.5 FREEMAIL_FROM Sender email is commonly abused enduser mail provider [jcdra1[at]gmail.com]
0.0 SPF_HELO_NONE SPF: HELO does not publish an SPF Record
0.0 DKIM_ADSP_CUSTOM_MED No valid author signature, adsp_override is CUSTOM_MED
0.0 FREEMAIL_FORGED_FROMDOMAIN 2nd level domains in From and EnvelopeFrom freemail headers are different
0.4 RDNS_DYNAMIC Delivered to internal network by host with dynamic-looking rDNS
1.2 NML_ADSP_CUSTOM_MED ADSP custom_med hit, and not from a mailing list
0.3 KHOP_HELO_FCRDNS Relay HELO differs from its IP's reverse DNS
2.0 SPOOFED_FREEMAIL No description available.
1.0 TO_NO_BRKTS_PCNT To: lacks brackets + percentage
I would suggest to fix that before resubmitting.
> ---
> drivers/hwmon/Kconfig | 10 +
> drivers/hwmon/Makefile | 1 +
> drivers/hwmon/aht10.c | 405 +++++++++++++++++++++++++++++++++++++++++
Documentation is missing.
> 3 files changed, 416 insertions(+)
> create mode 100644 drivers/hwmon/aht10.c
>
> diff --git a/drivers/hwmon/Kconfig b/drivers/hwmon/Kconfig
> index 288ae9f63588..96bad243d729 100644
> --- a/drivers/hwmon/Kconfig
> +++ b/drivers/hwmon/Kconfig
> @@ -257,6 +257,16 @@ config SENSORS_ADT7475
> This driver can also be built as a module. If so, the module
> will be called adt7475.
>
> +config SENSORS_AHT10
> + tristate "Aosong AHT10"
> + depends on I2C
> + help
> + If you say yes here, you get support for the Aosong AHT10
> + temperature and humidity sensors
> +
> + This driver can also be built as a module. If so, the module
> + will be called aht10.
> +
> config SENSORS_AS370
> tristate "Synaptics AS370 SoC hardware monitoring driver"
> help
> diff --git a/drivers/hwmon/Makefile b/drivers/hwmon/Makefile
> index 3e32c21f5efe..6cb44d54e628 100644
> --- a/drivers/hwmon/Makefile
> +++ b/drivers/hwmon/Makefile
> @@ -45,6 +45,7 @@ obj-$(CONFIG_SENSORS_ADT7411) += adt7411.o
> obj-$(CONFIG_SENSORS_ADT7462) += adt7462.o
> obj-$(CONFIG_SENSORS_ADT7470) += adt7470.o
> obj-$(CONFIG_SENSORS_ADT7475) += adt7475.o
> +obj-$(CONFIG_SENSORS_AHT10) += aht10.o
> obj-$(CONFIG_SENSORS_AMD_ENERGY) += amd_energy.o
> obj-$(CONFIG_SENSORS_APPLESMC) += applesmc.o
> obj-$(CONFIG_SENSORS_ARM_SCMI) += scmi-hwmon.o
> diff --git a/drivers/hwmon/aht10.c b/drivers/hwmon/aht10.c
> new file mode 100644
> index 000000000000..1eeddce02ae9
> --- /dev/null
> +++ b/drivers/hwmon/aht10.c
> @@ -0,0 +1,405 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +/*
> + * aht10.c - Linux hwmon driver for AHT10 I2C Temperature and Humidity sensor
> + * Copyright (C) 2020 Johannes Cornelis Draaijer
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/i2c.h>
> +#include <linux/mutex.h>
> +#include <linux/delay.h>
> +#include <linux/ktime.h>
> +#include <linux/err.h>
> +#include <asm/div64.h>
> +#include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
Alphabetic include file order, please.
> +
> +#define AHT10_ADDR 0x38
> +
> +// Delays
> +
> +#define AHT10_POWERON_USEC_DELAY 40000
> +#define AHT10_MEAS_USEC_DELAY 80000
> +#define AHT10_CMD_USEC_DELAY 350000
> +#define AHT10_USEC_DELAY_OFFSET 100000
Please use
#define<space>XXX<tab>VALUE
> +
> +// Command bytes
Please no C++ style comments (except for the SPDX identifier)
> +
> +#define AHT10_CMD_INIT 0b11100001
> +#define AHT10_CMD_MEAS 0b10101100
> +#define AHT10_CMD_RST 0b10111010
> +
> +// Flags in the answer byte/command
> +
> +#define AHT10_RESP_ERROR 0xFF
> +
> +#define AHT10_CAL_ENABLED (1u << 3u)
> +#define AHT10_BUSY (1u << 7u)
> +#define AHT10_MODE_NOR (0b11u << 5u)
> +#define AHT10_MODE_CYC (0b01u << 5u)
> +#define AHT10_MODE_CMD (0b10u << 5u)
> +
Please use bit operations where possible.,
> +#define AHT10_MAX_POLL_INTERVAL_LEN 30
> +
> +// Full commands
> +
> +const u8 cmd_init[] = {AHT10_CMD_INIT, AHT10_CAL_ENABLED | AHT10_MODE_CYC,
> + 0x00};
> +const u8 cmd_meas[] = {AHT10_CMD_MEAS, 0x33, 0x00};
> +const u8 cmd_rst[] = {AHT10_CMD_RST, 0x00, 0x00};
> +
> +struct aht10_measurement {
> + u8 data[6];
Only used within a function and thus pointless.
> + u8 status;
Not used at all.
> + int temperature;
> + int humidity;
Fold into struct aht10_data.
> +};
> +
> +/**
> + * struct aht10_data - All the data required to operate an AHT10 chip
> + * @client: the i2c client associated with the AHT10
> + * @lock: a mutex that is used to prevent parallel access to the
> + * i2c client
> + * @initialized: whether or not the AHT10 has been initialized
> + * @current_measurement: the last-measured values of the AHT10
> + * @poll_interval: the minimum poll interval
> + * While the poll rate is not 100% necessary,
> + * the datasheet recommends that a measurement
> + * is not performed more too often to prevent
> + * the chip from "heating up". If it's
> + * unwanted, it can be ignored by setting
> + * it to 0.
> + */
> +
> +struct aht10_data {
> + struct i2c_client *client;
> + struct mutex lock;
> + int initialized;
Only set and never used, and thus pointless.
> + struct aht10_measurement current_measurement;
> + ktime_t poll_interval;
> + ktime_t previous_poll_time;
> +};
> +
> +
Please no double empty lines. Having said that, checkpatch --strict reports:
total: 0 errors, 5 warnings, 17 checks, 428 lines checked
Please fix.
> +/**
> + * aht10_init() - Initialize an AHT10 chip
> + * @client: the i2c client associated with the AHT10
> + * @data: the data associated with this AHT10 chip
> + * Return: 0 if succesfull, 1 if not
> + */
> +static int aht10_init(struct i2c_client *client, struct aht10_data *data)
> +{
> + struct mutex *mutex = &data->lock;
Unnecessary variable.
> +
> + int res;
> + u8 status;
> +
> + mutex_lock(mutex);
> +
Unnecessary lock. This is the init function. It won't be called
multiple times in parallel.
> + usleep_range(AHT10_POWERON_USEC_DELAY, AHT10_POWERON_USEC_DELAY +
> + AHT10_USEC_DELAY_OFFSET);
Pointless delay.
> +
> + i2c_master_send(client, cmd_init, 3);
> +
> + usleep_range(AHT10_CMD_USEC_DELAY, AHT10_CMD_USEC_DELAY +
> + AHT10_USEC_DELAY_OFFSET);
> +
> + res = i2c_master_recv(client, &status, 1);
> +
> + if (res != 1) {
> + mutex_unlock(mutex);
> + return 1;
Return standard error codes. Everywhere.
> + }
> +
> + data->initialized = 1;
> +
> + if (status & AHT10_BUSY)
> + pr_warn("AHT10 busy flag is enabled! Is another program already using the AHT10?\n");
If this is a concern, return -EBUSY and exit with error.
> +
> + mutex_unlock(mutex);
> + return 0;
> +}
> +
> +/**
> + * aht10_read_data() - read and parse the raw data from the AHT10
> + * @client: the i2c client associated with the AHT10
> + * @aht10_data: the struct aht10_data to use for the lock
> + * @aht10_measurement: the struct aht10_measurement to store the raw
> + * data and parsed values in
> + * Return: 0 if succesfull, 1 if not
> + */
> +static int aht10_read_data(struct i2c_client *client,
> + struct aht10_data *aht10_data,
> + struct aht10_measurement *measurement)
> +{
> + u32 temp, hum;
> + int hum_i, temp_i;
> + int res;
> + struct mutex *mutex = &aht10_data->lock;
> + int was_locked = mutex_is_locked(mutex);
> + u8 *raw_data = measurement->data;
> +
> + mutex_lock(mutex);
> + if (!was_locked) {
This is both unnecessary and unsafe. Check and update
previous_poll_time from within the lock instead.
> + i2c_master_send(client, cmd_meas, 3);
> + usleep_range(AHT10_MEAS_USEC_DELAY,
> + AHT10_MEAS_USEC_DELAY + AHT10_USEC_DELAY_OFFSET);
> +
> + res = i2c_master_recv(client, raw_data, 6);
> +
> + if (res != 6) {
> + mutex_unlock(mutex);
> + pr_warn("Did not receive 6 bytes from AHT10!\n");
Please no such noise.
> + return 1;
> + }
> +
> + temp = ((u32) (raw_data[3] & 0x0Fu) << 16u) | ((u32) raw_data[4] << 8u) | raw_data[5];
> + hum = ((u32) raw_data[1] << 12u) | ((u32) raw_data[2] << 4u) | (raw_data[3] & 0xF0u >> 4u);
> +
> + /*
> + * Avoid doing float arithmetic, while trying to preserve
> + * precision. There must be a better way to do this (or
> + * by using 64 bit values)
> + */
Pointless comment. Then implement it.
> +
> + temp = temp * 200;
> + temp = temp >> 10u;
> + temp = temp * 100;
> + temp = temp >> 10u;
> +
> + hum = hum * 100;
> + hum = hum >> 10u;
> + hum = hum * 100;
> + hum = hum >> 10u;
> +
> + temp_i = temp - 5000;
> + hum_i = hum;
> +
> + measurement->temperature = temp_i;
> + measurement->humidity = hum_i;
> + }
> + mutex_unlock(mutex);
> + return 0;
> +}
> +
> +/**
> + * aht10_check_and_set_polltime() - check if the minimum poll interval has
> + * expired, and if so set the previous
> + * poll time
> + * @data: what time to compare (and possibly set)
> + * Return: 1 if the minimum poll interval has expired, 0 if not
> + */
> +static int aht10_check_and_set_polltime(struct aht10_data *data)
> +{
> + ktime_t current_time = ktime_get_boottime();
> + ktime_t difference = ktime_sub(current_time,
> + data->previous_poll_time);
> + if (ktime_to_us(difference) >=
> + ktime_to_us(data->poll_interval)) {
Unnecessary line split, and really bad alignment. Also way too complex.
Use ktime_after() or similar instead.
> + data->previous_poll_time = current_time;
> + return 1;
> + }
> + return 0;
> +}
> +
> +/**
> + * temperature_show() - show the temperature in Celcius
> + */
> +static ssize_t temperature_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + int bytes_written;
> + struct aht10_data *data = dev_get_drvdata(dev);
> + struct i2c_client *client = data->client;
> + struct aht10_measurement *measurement = &data->current_measurement;
> +
> + if (aht10_check_and_set_polltime(data))
> + aht10_read_data(client, data, measurement);
> +
> + bytes_written = sprintf(buf, "%d", measurement->temperature * 10);
> + return bytes_written;
> +}
> +
> +
> +/**
> + * humidity_show() - show the relative humidity in %H
> + */
> +static ssize_t humidity_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + int bytes_written;
> + struct aht10_data *data = dev_get_drvdata(dev);
> + struct i2c_client *client = data->client;
> + struct aht10_measurement *measurement = &data->current_measurement;
> +
> + if (aht10_check_and_set_polltime(data))
> + aht10_read_data(client, data, measurement);
> +
> + bytes_written = sprintf(buf, "%d", measurement->humidity * 10);
> + return bytes_written;
> +}
> +
> +/**
> + * reset_store() - reset the ATH10
> + */
> +static ssize_t reset_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + // TODO
> + return count;
Not acceptable.
> +}
> +
> +/**
> + * min_poll_interval_show() - show the minimum poll interval
> + * in milliseconds
> + */
> +static ssize_t min_poll_interval_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct aht10_data *data = dev_get_drvdata(dev);
> + int bytes_written;
> + u64 usec = ktime_to_us(data->poll_interval);
> +
> + do_div(usec, USEC_PER_MSEC);
> + bytes_written = sprintf(buf, "%lld", usec);
> + return bytes_written;
> +}
> +
> +/**
> + * min_poll_interval_store() - store the given minimum poll interval.
> + * Input in milliseconds
> + */
> +static ssize_t min_poll_interval_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct aht10_data *data = dev_get_drvdata(dev);
> + int i;
> + u64 msecs;
> + int res;
> +
> + char null_terminated[AHT10_MAX_POLL_INTERVAL_LEN + 1];
> +
> + if (count > AHT10_MAX_POLL_INTERVAL_LEN) {
> + pr_warn("AHT10: Warning! Input too long. Max characters: %d\n",
> + AHT10_MAX_POLL_INTERVAL_LEN);
> + return count;
> + }
> +
> + for (i = 0; i < count && i < AHT10_MAX_POLL_INTERVAL_LEN; i++)
> + null_terminated[i] = buf[i];
> +
> + null_terminated[i] = 0;
> +
> + res = kstrtoull(null_terminated, 10, &msecs);
What is the point of this ? kstrtoull() works directly on buf.
> +
> + if (res) {
> + pr_warn("AHT10: Warning! Invalid input.\n");
Please no such error messages. They can be used to clog the log.
Return standard error codes....
> + return count;
... and don't ignore errors.
> + }
> +
> + data->poll_interval = ms_to_ktime(msecs);
> + return count;
> +}
> +
> +static SENSOR_DEVICE_ATTR(reset, 0200, NULL, reset_store, 0);
> +static SENSOR_DEVICE_ATTR(temp1_input, 0444, temperature_show, NULL, 0);
> +static SENSOR_DEVICE_ATTR(humidity1_input, 0444, humidity_show, NULL, 0);
> +static SENSOR_DEVICE_ATTR(min_poll_interval, 0644, min_poll_interval_show,
> + min_poll_interval_store, 0);
Please use standard attributes.
> +
> +static struct attribute *aht10_attrs[] = {
> + &sensor_dev_attr_reset.dev_attr.attr,
> + &sensor_dev_attr_temp1_input.dev_attr.attr,
> + &sensor_dev_attr_humidity1_input.dev_attr.attr,
> + &sensor_dev_attr_min_poll_interval.dev_attr.attr,
> + NULL,
> +};
> +
> +ATTRIBUTE_GROUPS(aht10);
> +
> +static int aht10_probe(struct i2c_client *client,
> + const struct i2c_device_id *aht10_id)
> +{
> + struct device *device = &client->dev;
> + struct device *hwmon_dev;
> + struct i2c_adapter *adapter = client->adapter;
> + struct aht10_data *data;
> + const struct attribute_group **attribute_groups = aht10_groups;
> + int res = 0;
> +
> + if (!i2c_check_functionality(adapter, I2C_FUNC_I2C))
> + return 0;
> +
> + if (client->addr != AHT10_ADDR)
> + return 0;
> +
> + data = devm_kzalloc(device, sizeof(*data), GFP_KERNEL);
> +
Unnecessary empty line. In general, please no empty line between assignments
and value checks.
> + if (!data)
> + return -ENOMEM;
> +
> + data->poll_interval = ns_to_ktime((u64) 10000 * NSEC_PER_MSEC);
> + data->previous_poll_time = ns_to_ktime(0);
> + data->client = client;
> +
> + i2c_set_clientdata(client, data);
> +
> + mutex_init(&data->lock);
> +
> + res = aht10_init(client, data);
> +
> + if (res)
> + return 2;
> +
Please use standard error codes.
> + hwmon_dev = devm_hwmon_device_register_with_groups(device,
> + client->name,
> + data,
> + attribute_groups);
New drivers shall use devm_hwmon_device_register_with_info().
> +
> + pr_info("AHT10 was detected and registered\n");
> + return PTR_ERR_OR_ZERO(hwmon_dev);
> +}
> +
> +static int aht10_remove(struct i2c_client *client)
> +{
> + if (client->addr != AHT10_ADDR)
> + return 0;
> +
> + pr_info("AHT10 was removed\n");
> + return 0;
> +}
Pointless remove function.
> +
> +static const struct i2c_device_id aht10_id[] = {
> + { "aht10", 0 },
> + { },
> +};
> +MODULE_DEVICE_TABLE(i2c, aht10_id);
> +
> +static const struct of_device_id aht10_of_match[] = {
> + { .compatible = "aht10", },
Needs to be aosong,aht10 or similar (ie vendor prefix needed),
and needs to be documented.
> +};
> +> +static struct i2c_driver aht10_driver = {
> + .driver = {
> + .name = "aht10",
> + .of_match_table = aht10_of_match,
This implies (unnecessary) dependency on devicetree. Please
use of_match_ptr().
> + },
> + .probe = aht10_probe,
> + .remove = aht10_remove,
> + .id_table = aht10_id,
> +};
> +
> +module_i2c_driver(aht10_driver);
> +
> +MODULE_AUTHOR("Johannes Draaijer <jcdra1@xxxxxxxxx>");
> +MODULE_DESCRIPTION("AHT10 Temperature and Humidity sensor driver");
> +MODULE_VERSION("1.0");
> +MODULE_LICENSE("GPL v2");
>
