Re: [PATCH] hwmon: Add AHT10 Temperature and Humidity Sensor Driver

[Johannes Cornelis Draaijer <jcdra1@gmail.com>]

Thank you very much for the insightful and quick response. 
I think I've fixed my mail issues now (once more, thanks for 
pointing that out). I will fix the issues that you've listed and
resubmit sometime during this week.

On Sun, Dec 13, 2020 at 03:10:52PM -0800, Guenter Roeck wrote:
> 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@gmail.com>
> 
> 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@gmail.com>");
> > +MODULE_DESCRIPTION("AHT10 Temperature and Humidity sensor driver");
> > +MODULE_VERSION("1.0");
> > +MODULE_LICENSE("GPL v2");
> > 
>