[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <8e45875a-8af2-4409-8ea8-255281f97e68@roeck-us.net>
Date: Sun, 25 Aug 2024 17:15:41 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Antoni Pokusinski <apokusinski01@...il.com>, jdelvare@...e.com
Cc: linux-kernel@...r.kernel.org, linux-hwmon@...r.kernel.org
Subject: Re: [PATCH v2] hwmon: (sht4x): add heater support
On 8/25/24 03:09, Antoni Pokusinski wrote:
> Add support for manipulating the internal heater of sht4x devices.
> The heater may be used to remove condensed water on the sensor surface
> which disturbs the relative humidity measurements.
>
Where is this seen in practice ? "may be" does not mean that the problem
is actively seen.
> The heater can operate at three heating levels (20, 110 or 200
> milliwatts). Also, two heating durations may be selected (0.1 or 1s).
> Once the heating time elapses the heater is automatically switched off.
>
> Signed-off-by: Antoni Pokusinski <apokusinski01@...il.com>
> ---
> Changes since v1:
> * explain the use case of the new attributes set
> * heater_enable attr: make it write-only
> * heater_enable_store: define cmd as u8 instead of u8*
> * heater_enable_store: remove unreachable data path
> * heater_enable_store: remove unnecessary lock
> * heater_enable_store: call i2c_master_send only if status==true
> * define attributes as DEVICE_ATTR_* instead of SENSOR_DEVICE_ATTR_*
> ---
> Documentation/hwmon/sht4x.rst | 11 +++
> drivers/hwmon/sht4x.c | 126 +++++++++++++++++++++++++++++++++-
> 2 files changed, 136 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/hwmon/sht4x.rst b/Documentation/hwmon/sht4x.rst
> index daf21e763425..3ff6c08d8267 100644
> --- a/Documentation/hwmon/sht4x.rst
> +++ b/Documentation/hwmon/sht4x.rst
> @@ -42,4 +42,15 @@ humidity1_input Measured humidity in %H
> update_interval The minimum interval for polling the sensor,
> in milliseconds. Writable. Must be at least
> 2000.
> +heater_power The requested heater power, in milliwatts.
> + Available values: 20, 110, 200 (default: 200).
> +heater_time The requested operating time of the heater,
> + in milliseconds.
> + Available values: 100, 1000 (default 1000).
> +heater_enable Enable the heater with the selected power
> + and for the selected time in order to remove
> + condensed water from the sensor surface. Write-only.
> +
> + - 0: turn off
Not really. As implemented, this does not turn off the heater, it does nothing.
If that is what you want' you'd have to send a command "turn off heater immediately"
if such a command exists.
> + - 1: turn on
> =============== ============================================
> diff --git a/drivers/hwmon/sht4x.c b/drivers/hwmon/sht4x.c
> index b8916d2735b5..75e092f9b80e 100644
> --- a/drivers/hwmon/sht4x.c
> +++ b/drivers/hwmon/sht4x.c
> @@ -11,6 +11,7 @@
> #include <linux/crc8.h>
> #include <linux/delay.h>
> #include <linux/hwmon.h>
> +#include <linux/hwmon-sysfs.h>
> #include <linux/i2c.h>
> #include <linux/jiffies.h>
> #include <linux/module.h>
> @@ -31,6 +32,12 @@
> */
> #define SHT4X_CMD_MEASURE_HPM 0b11111101
> #define SHT4X_CMD_RESET 0b10010100
> +#define SHT4X_CMD_HEATER_20_1 0b00011110
> +#define SHT4X_CMD_HEATER_20_01 0b00010101
> +#define SHT4X_CMD_HEATER_110_1 0b00101111
> +#define SHT4X_CMD_HEATER_110_01 0b00100100
> +#define SHT4X_CMD_HEATER_200_1 0b00111001
> +#define SHT4X_CMD_HEATER_200_01 0b00110010
>
> #define SHT4X_CMD_LEN 1
> #define SHT4X_CRC8_LEN 1
> @@ -54,6 +61,8 @@ DECLARE_CRC8_TABLE(sht4x_crc8_table);
> * @last_updated: the previous time that the SHT4X was polled
> * @temperature: the latest temperature value received from the SHT4X
> * @humidity: the latest humidity value received from the SHT4X
> + * @heater_power: the power at which the heater will be started
> + * @heater_time: the time for which the heater will remain turned on
> */
> struct sht4x_data {
> struct i2c_client *client;
> @@ -63,6 +72,8 @@ struct sht4x_data {
> long last_updated; /* in jiffies */
> s32 temperature;
> s32 humidity;
> + u32 heater_power; /* in milli-watts */
> + u32 heater_time; /* in milli-seconds */
> };
>
> /**
> @@ -215,6 +226,117 @@ static int sht4x_hwmon_write(struct device *dev, enum hwmon_sensor_types type,
> }
> }
>
> +static ssize_t heater_enable_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf,
> + size_t count)
> +{
> + struct sht4x_data *data = dev_get_drvdata(dev);
> + bool status;
> + ssize_t ret;
> + u8 cmd;
> +
> + ret = kstrtobool(buf, &status);
> + if (ret)
> + return ret;
> +
> + if (status) {
> + if (data->heater_power == 20) {
> + if (data->heater_time == 100)
> + cmd = SHT4X_CMD_HEATER_20_01;
> + else /* data->heater_time == 1000 */
> + cmd = SHT4X_CMD_HEATER_20_1;
> + } else if (data->heater_power == 110) {
> + if (data->heater_time == 100)
> + cmd = SHT4X_CMD_HEATER_110_01;
> + else /* data->heater_time == 1000 */
> + cmd = SHT4X_CMD_HEATER_110_1;
> + } else if (data->heater_power == 200) {
Unnecessary "if". Due to the limitations of static checkers, those
may wrongly conclude that "cmd" is not initialized when passed to
i2c_master_send().
> + if (data->heater_time == 100)
> + cmd = SHT4X_CMD_HEATER_200_01;
> + else /* data->heater_time == 1000 */
> + cmd = SHT4X_CMD_HEATER_200_1;
> + }
> +
> + ret = i2c_master_send(data->client, &cmd, SHT4X_CMD_LEN);
> + }
> +
> + return ret; > +}
> +
> +static ssize_t heater_power_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct sht4x_data *data = dev_get_drvdata(dev);
> +
> + return sysfs_emit(buf, "%u\n", data->heater_power);
> +}
> +
> +static ssize_t heater_power_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf,
> + size_t count)
> +{
> + struct sht4x_data *data = dev_get_drvdata(dev);
> + u32 power;
> + ssize_t ret;
> +
> + ret = kstrtou32(buf, 10, &power);
> + if (ret)
> + return ret;
> +
> + if (power != 20 && power != 110 && power != 200)
> + return -EINVAL;
> +
> + data->heater_power = power;
> +
> + return count;
> +}
> +
> +static ssize_t heater_time_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct sht4x_data *data = dev_get_drvdata(dev);
> +
> + return sysfs_emit(buf, "%u\n", data->heater_time);
> +}
> +
> +static ssize_t heater_time_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf,
> + size_t count)
> +{
> + struct sht4x_data *data = dev_get_drvdata(dev);
> + u32 time;
> + ssize_t ret;
> +
> + ret = kstrtou32(buf, 10, &time);
> + if (ret)
> + return ret;
> +
> + if (time != 100 && time != 1000)
> + return -EINVAL;
> +
> + data->heater_time = time;
> +
> + return count;
> +}
> +
> +static DEVICE_ATTR_WO(heater_enable);
> +static DEVICE_ATTR_RW(heater_power);
> +static DEVICE_ATTR_RW(heater_time);
> +
> +static struct attribute *sht4x_attrs[] = {
> + &dev_attr_heater_enable.attr,
> + &dev_attr_heater_power.attr,
> + &dev_attr_heater_time.attr,
> + NULL
> +};
> +
> +ATTRIBUTE_GROUPS(sht4x);
> +
> static const struct hwmon_channel_info * const sht4x_info[] = {
> HWMON_CHANNEL_INFO(chip, HWMON_C_UPDATE_INTERVAL),
> HWMON_CHANNEL_INFO(temp, HWMON_T_INPUT),
> @@ -255,6 +377,8 @@ static int sht4x_probe(struct i2c_client *client)
>
> data->update_interval = SHT4X_MIN_POLL_INTERVAL;
> data->client = client;
> + data->heater_power = 200;
> + data->heater_time = 1000;
>
> mutex_init(&data->lock);
>
> @@ -270,7 +394,7 @@ static int sht4x_probe(struct i2c_client *client)
> client->name,
> data,
> &sht4x_chip_info,
> - NULL);
> + sht4x_groups);
>
> return PTR_ERR_OR_ZERO(hwmon_dev);
> }
Powered by blists - more mailing lists