* Re: [PATCH] hwmon: Add AHT10 Temperature and Humidity Sensor Driver
2020-12-13 23:10 ` [PATCH] hwmon: Add AHT10 Temperature and Humidity Sensor Driver Guenter Roeck
@ 2020-12-14 9:00 ` Johannes Cornelis Draaijer
0 siblings, 0 replies; 2+ messages in thread
From: Johannes Cornelis Draaijer @ 2020-12-14 9:00 UTC (permalink / raw)
To: Guenter Roeck; +Cc: linux-hwmon
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");
> >
>
^ permalink raw reply [flat|nested] 2+ messages in thread