* [PATCH 1/3] hwmon: (sht3x)remove sht3x_platform_data
@ 2023-06-14 6:24 JuenKit Yip
2023-06-14 10:05 ` kernel test robot
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: JuenKit Yip @ 2023-06-14 6:24 UTC (permalink / raw)
To: linux, jdelvare; +Cc: linux-hwmon, linux-kernel, JuenKit Yip
Since no in-tree driver supports it, the sht3x_platform_data was
removed.
- "blocking_io" property and its related code have been removed, and
Single-Shot mode should be blocking in default.
- "high-precision" property has been replaced to "repeatability" for
matching datasheet.
Signed-off-by: JuenKit Yip <JuenKit_Yip@hotmail.com>
---
Documentation/hwmon/sht3x.rst | 12 +++++------
drivers/hwmon/sht3x.c | 32 ++++++++++++-----------------
include/linux/platform_data/sht3x.h | 15 --------------
3 files changed, 18 insertions(+), 41 deletions(-)
delete mode 100644 include/linux/platform_data/sht3x.h
diff --git a/Documentation/hwmon/sht3x.rst b/Documentation/hwmon/sht3x.rst
index 95a850d5b..2c87c8f58 100644
--- a/Documentation/hwmon/sht3x.rst
+++ b/Documentation/hwmon/sht3x.rst
@@ -28,28 +28,26 @@ The device communicates with the I2C protocol. Sensors can have the I2C
addresses 0x44 or 0x45, depending on the wiring. See
Documentation/i2c/instantiating-devices.rst for methods to instantiate the device.
-There are two options configurable by means of sht3x_platform_data:
+This driver supports block and non-block mode:
-1. blocking (pull the I2C clock line down while performing the measurement) or
+ blocking (pull the I2C clock line down while performing the measurement) or
non-blocking mode. Blocking mode will guarantee the fastest result but
the I2C bus will be busy during that time. By default, non-blocking mode
is used. Make sure clock-stretching works properly on your device if you
want to use blocking mode.
-2. high or low accuracy. High accuracy is used by default and using it is
- strongly recommended.
The sht3x sensor supports a single shot mode as well as 5 periodic measure
modes, which can be controlled with the update_interval sysfs interface.
The allowed update_interval in milliseconds are as follows:
- ===== ======= ====================
- 0 single shot mode
+ ===== ======= ==========================
+ 0 single shot mode(blocking)
2000 0.5 Hz periodic measurement
1000 1 Hz periodic measurement
500 2 Hz periodic measurement
250 4 Hz periodic measurement
100 10 Hz periodic measurement
- ===== ======= ====================
+ ===== ======= ==========================
In the periodic measure mode, the sensor automatically triggers a measurement
with the configured update interval on the chip. When a temperature or humidity
diff --git a/drivers/hwmon/sht3x.c b/drivers/hwmon/sht3x.c
index 8305e44d9..5bc0001b1 100644
--- a/drivers/hwmon/sht3x.c
+++ b/drivers/hwmon/sht3x.c
@@ -20,13 +20,12 @@
#include <linux/module.h>
#include <linux/slab.h>
#include <linux/jiffies.h>
-#include <linux/platform_data/sht3x.h>
-/* commands (high precision mode) */
+/* commands (high repeatability mode) */
static const unsigned char sht3x_cmd_measure_blocking_hpm[] = { 0x2c, 0x06 };
static const unsigned char sht3x_cmd_measure_nonblocking_hpm[] = { 0x24, 0x00 };
-/* commands (low power mode) */
+/* commands (low repeatability mode) */
static const unsigned char sht3x_cmd_measure_blocking_lpm[] = { 0x2c, 0x10 };
static const unsigned char sht3x_cmd_measure_nonblocking_lpm[] = { 0x24, 0x16 };
@@ -69,9 +68,14 @@ enum sht3x_limits {
limit_min_hyst,
};
+enum sht3x_repeatability {
+ low_repeatability,
+ high_repeatability,
+};
+
DECLARE_CRC8_TABLE(sht3x_crc8_table);
-/* periodic measure commands (high precision mode) */
+/* periodic measure commands (high repeatability mode) */
static const char periodic_measure_commands_hpm[][SHT3X_CMD_LENGTH] = {
/* 0.5 measurements per second */
{0x20, 0x32},
@@ -85,7 +89,7 @@ static const char periodic_measure_commands_hpm[][SHT3X_CMD_LENGTH] = {
{0x27, 0x37},
};
-/* periodic measure commands (low power mode) */
+/* periodic measure commands (low repeatability mode) */
static const char periodic_measure_commands_lpm[][SHT3X_CMD_LENGTH] = {
/* 0.5 measurements per second */
{0x20, 0x2f},
@@ -132,12 +136,11 @@ struct sht3x_data {
struct mutex data_lock; /* lock for updating driver data */
u8 mode;
+ enum sht3x_repeatability repeatability;
const unsigned char *command;
u32 wait_time; /* in us*/
unsigned long last_update; /* last update in periodic mode*/
- struct sht3x_platform_data setup;
-
/*
* cached values for temperature and humidity and limits
* the limits arrays have the following order:
@@ -441,13 +444,8 @@ static void sht3x_select_command(struct sht3x_data *data)
if (data->mode > 0) {
data->command = sht3x_cmd_measure_periodic_mode;
data->wait_time = 0;
- } else if (data->setup.blocking_io) {
- data->command = data->setup.high_precision ?
- sht3x_cmd_measure_blocking_hpm :
- sht3x_cmd_measure_blocking_lpm;
- data->wait_time = 0;
} else {
- if (data->setup.high_precision) {
+ if (data->repeatability == high_repeatability) {
data->command = sht3x_cmd_measure_nonblocking_hpm;
data->wait_time = SHT3X_NONBLOCKING_WAIT_TIME_HPM;
} else {
@@ -595,7 +593,7 @@ static ssize_t update_interval_store(struct device *dev,
}
if (mode > 0) {
- if (data->setup.high_precision)
+ if (data->repeatability == high_repeatability)
command = periodic_measure_commands_hpm[mode - 1];
else
command = periodic_measure_commands_lpm[mode - 1];
@@ -690,16 +688,12 @@ static int sht3x_probe(struct i2c_client *client)
if (!data)
return -ENOMEM;
- data->setup.blocking_io = false;
- data->setup.high_precision = true;
+ data->repeatability = high_repeatability;
data->mode = 0;
data->last_update = jiffies - msecs_to_jiffies(3000);
data->client = client;
crc8_populate_msb(sht3x_crc8_table, SHT3X_CRC8_POLYNOMIAL);
- if (client->dev.platform_data)
- data->setup = *(struct sht3x_platform_data *)dev->platform_data;
-
sht3x_select_command(data);
mutex_init(&data->i2c_lock);
diff --git a/include/linux/platform_data/sht3x.h b/include/linux/platform_data/sht3x.h
deleted file mode 100644
index 14680d2a9..000000000
--- a/include/linux/platform_data/sht3x.h
+++ /dev/null
@@ -1,15 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-or-later */
-/*
- * Copyright (C) 2016 Sensirion AG, Switzerland
- * Author: David Frey <david.frey@sensirion.com>
- * Author: Pascal Sachs <pascal.sachs@sensirion.com>
- */
-
-#ifndef __SHT3X_H_
-#define __SHT3X_H_
-
-struct sht3x_platform_data {
- bool blocking_io;
- bool high_precision;
-};
-#endif /* __SHT3X_H_ */
--
2.30.2
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] hwmon: (sht3x)remove sht3x_platform_data
2023-06-14 6:24 [PATCH 1/3] hwmon: (sht3x)remove sht3x_platform_data JuenKit Yip
@ 2023-06-14 10:05 ` kernel test robot
2023-06-14 12:57 ` Guenter Roeck
2023-06-14 17:15 ` kernel test robot
2 siblings, 0 replies; 9+ messages in thread
From: kernel test robot @ 2023-06-14 10:05 UTC (permalink / raw)
To: JuenKit Yip, linux, jdelvare
Cc: llvm, oe-kbuild-all, linux-hwmon, linux-kernel, JuenKit Yip
Hi JuenKit,
kernel test robot noticed the following build warnings:
[auto build test WARNING on groeck-staging/hwmon-next]
[also build test WARNING on linus/master v6.4-rc6 next-20230614]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/JuenKit-Yip/hwmon-sht3x-add-medium-repeatability-support/20230614-143100
base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
patch link: https://lore.kernel.org/r/DB4PR10MB6261D79FE16EC2BBD5316B91925AA%40DB4PR10MB6261.EURPRD10.PROD.OUTLOOK.COM
patch subject: [PATCH 1/3] hwmon: (sht3x)remove sht3x_platform_data
config: hexagon-randconfig-r045-20230612 (https://download.01.org/0day-ci/archive/20230614/202306141736.b9bPRO0Z-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project.git 4a5ac14ee968ff0ad5d2cc1ffa0299048db4c88a)
reproduce (this is a W=1 build):
mkdir -p ~/bin
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
git remote add groeck-staging https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git
git fetch groeck-staging hwmon-next
git checkout groeck-staging/hwmon-next
b4 shazam https://lore.kernel.org/r/DB4PR10MB6261D79FE16EC2BBD5316B91925AA@DB4PR10MB6261.EURPRD10.PROD.OUTLOOK.COM
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=hexagon olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang ~/bin/make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/hwmon/
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306141736.b9bPRO0Z-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from drivers/hwmon/sht3x.c:17:
In file included from include/linux/i2c.h:19:
In file included from include/linux/regulator/consumer.h:35:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:9:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
547 | val = __raw_readb(PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
560 | val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
| ^
In file included from drivers/hwmon/sht3x.c:17:
In file included from include/linux/i2c.h:19:
In file included from include/linux/regulator/consumer.h:35:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:9:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
573 | val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
| ~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
| ^
In file included from drivers/hwmon/sht3x.c:17:
In file included from include/linux/i2c.h:19:
In file included from include/linux/regulator/consumer.h:35:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:13:
In file included from include/linux/cgroup.h:26:
In file included from include/linux/kernel_stat.h:9:
In file included from include/linux/interrupt.h:11:
In file included from include/linux/hardirq.h:11:
In file included from ./arch/hexagon/include/generated/asm/hardirq.h:1:
In file included from include/asm-generic/hardirq.h:17:
In file included from include/linux/irq.h:20:
In file included from include/linux/io.h:13:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
584 | __raw_writeb(value, PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
594 | __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
604 | __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
| ~~~~~~~~~~ ^
>> drivers/hwmon/sht3x.c:25:28: warning: unused variable 'sht3x_cmd_measure_blocking_hpm' [-Wunused-const-variable]
25 | static const unsigned char sht3x_cmd_measure_blocking_hpm[] = { 0x2c, 0x06 };
| ^
>> drivers/hwmon/sht3x.c:29:28: warning: unused variable 'sht3x_cmd_measure_blocking_lpm' [-Wunused-const-variable]
29 | static const unsigned char sht3x_cmd_measure_blocking_lpm[] = { 0x2c, 0x10 };
| ^
8 warnings generated.
vim +/sht3x_cmd_measure_blocking_hpm +25 drivers/hwmon/sht3x.c
7c84f7f80d6fcea David Frey 2016-06-02 23
cecbab8bdd40311 JuenKit Yip 2023-06-14 24 /* commands (high repeatability mode) */
7c84f7f80d6fcea David Frey 2016-06-02 @25 static const unsigned char sht3x_cmd_measure_blocking_hpm[] = { 0x2c, 0x06 };
7c84f7f80d6fcea David Frey 2016-06-02 26 static const unsigned char sht3x_cmd_measure_nonblocking_hpm[] = { 0x24, 0x00 };
7c84f7f80d6fcea David Frey 2016-06-02 27
cecbab8bdd40311 JuenKit Yip 2023-06-14 28 /* commands (low repeatability mode) */
7c84f7f80d6fcea David Frey 2016-06-02 @29 static const unsigned char sht3x_cmd_measure_blocking_lpm[] = { 0x2c, 0x10 };
7c84f7f80d6fcea David Frey 2016-06-02 30 static const unsigned char sht3x_cmd_measure_nonblocking_lpm[] = { 0x24, 0x16 };
7c84f7f80d6fcea David Frey 2016-06-02 31
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] hwmon: (sht3x)remove sht3x_platform_data
2023-06-14 6:24 [PATCH 1/3] hwmon: (sht3x)remove sht3x_platform_data JuenKit Yip
2023-06-14 10:05 ` kernel test robot
@ 2023-06-14 12:57 ` Guenter Roeck
2023-06-14 15:02 ` JuenKit Yip
[not found] ` <01f88d7e-d461-8d73-6633-5b9060e42c37@hotmail.com>
2023-06-14 17:15 ` kernel test robot
2 siblings, 2 replies; 9+ messages in thread
From: Guenter Roeck @ 2023-06-14 12:57 UTC (permalink / raw)
To: JuenKit Yip, jdelvare; +Cc: linux-hwmon, linux-kernel
On 6/13/23 23:24, JuenKit Yip wrote:
> Since no in-tree driver supports it, the sht3x_platform_data was
> removed.
>
> - "blocking_io" property and its related code have been removed, and
> Single-Shot mode should be blocking in default.
>
> - "high-precision" property has been replaced to "repeatability" for
> matching datasheet.
>
That needs to be three patches.
> Signed-off-by: JuenKit Yip <JuenKit_Yip@hotmail.com>
> ---
> Documentation/hwmon/sht3x.rst | 12 +++++------
> drivers/hwmon/sht3x.c | 32 ++++++++++++-----------------
> include/linux/platform_data/sht3x.h | 15 --------------
> 3 files changed, 18 insertions(+), 41 deletions(-)
> delete mode 100644 include/linux/platform_data/sht3x.h
>
> diff --git a/Documentation/hwmon/sht3x.rst b/Documentation/hwmon/sht3x.rst
> index 95a850d5b..2c87c8f58 100644
> --- a/Documentation/hwmon/sht3x.rst
> +++ b/Documentation/hwmon/sht3x.rst
> @@ -28,28 +28,26 @@ The device communicates with the I2C protocol. Sensors can have the I2C
> addresses 0x44 or 0x45, depending on the wiring. See
> Documentation/i2c/instantiating-devices.rst for methods to instantiate the device.
>
> -There are two options configurable by means of sht3x_platform_data:
> +This driver supports block and non-block mode:
>
> -1. blocking (pull the I2C clock line down while performing the measurement) or
> + blocking (pull the I2C clock line down while performing the measurement) or
> non-blocking mode. Blocking mode will guarantee the fastest result but
> the I2C bus will be busy during that time. By default, non-blocking mode
> is used. Make sure clock-stretching works properly on your device if you
> want to use blocking mode.
> -2. high or low accuracy. High accuracy is used by default and using it is
> - strongly recommended.
>
> The sht3x sensor supports a single shot mode as well as 5 periodic measure
> modes, which can be controlled with the update_interval sysfs interface.
> The allowed update_interval in milliseconds are as follows:
>
> - ===== ======= ====================
> - 0 single shot mode
> + ===== ======= ==========================
> + 0 single shot mode(blocking)
> 2000 0.5 Hz periodic measurement
> 1000 1 Hz periodic measurement
> 500 2 Hz periodic measurement
> 250 4 Hz periodic measurement
> 100 10 Hz periodic measurement
> - ===== ======= ====================
> + ===== ======= ==========================
>
> In the periodic measure mode, the sensor automatically triggers a measurement
> with the configured update interval on the chip. When a temperature or humidity
> diff --git a/drivers/hwmon/sht3x.c b/drivers/hwmon/sht3x.c
> index 8305e44d9..5bc0001b1 100644
> --- a/drivers/hwmon/sht3x.c
> +++ b/drivers/hwmon/sht3x.c
> @@ -20,13 +20,12 @@
> #include <linux/module.h>
> #include <linux/slab.h>
> #include <linux/jiffies.h>
> -#include <linux/platform_data/sht3x.h>
>
> -/* commands (high precision mode) */
> +/* commands (high repeatability mode) */
> static const unsigned char sht3x_cmd_measure_blocking_hpm[] = { 0x2c, 0x06 };
> static const unsigned char sht3x_cmd_measure_nonblocking_hpm[] = { 0x24, 0x00 };
>
> -/* commands (low power mode) */
> +/* commands (low repeatability mode) */
> static const unsigned char sht3x_cmd_measure_blocking_lpm[] = { 0x2c, 0x10 };
> static const unsigned char sht3x_cmd_measure_nonblocking_lpm[] = { 0x24, 0x16 };
>
> @@ -69,9 +68,14 @@ enum sht3x_limits {
> limit_min_hyst,
> };
>
> +enum sht3x_repeatability {
> + low_repeatability,
> + high_repeatability,
> +};
> +
> DECLARE_CRC8_TABLE(sht3x_crc8_table);
>
> -/* periodic measure commands (high precision mode) */
> +/* periodic measure commands (high repeatability mode) */
> static const char periodic_measure_commands_hpm[][SHT3X_CMD_LENGTH] = {
> /* 0.5 measurements per second */
> {0x20, 0x32},
> @@ -85,7 +89,7 @@ static const char periodic_measure_commands_hpm[][SHT3X_CMD_LENGTH] = {
> {0x27, 0x37},
> };
>
> -/* periodic measure commands (low power mode) */
> +/* periodic measure commands (low repeatability mode) */
> static const char periodic_measure_commands_lpm[][SHT3X_CMD_LENGTH] = {
> /* 0.5 measurements per second */
> {0x20, 0x2f},
> @@ -132,12 +136,11 @@ struct sht3x_data {
> struct mutex data_lock; /* lock for updating driver data */
>
> u8 mode;
> + enum sht3x_repeatability repeatability;
> const unsigned char *command;
> u32 wait_time; /* in us*/
> unsigned long last_update; /* last update in periodic mode*/
>
> - struct sht3x_platform_data setup;
> -
> /*
> * cached values for temperature and humidity and limits
> * the limits arrays have the following order:
> @@ -441,13 +444,8 @@ static void sht3x_select_command(struct sht3x_data *data)
> if (data->mode > 0) {
> data->command = sht3x_cmd_measure_periodic_mode;
> data->wait_time = 0;
> - } else if (data->setup.blocking_io) {
> - data->command = data->setup.high_precision ?
> - sht3x_cmd_measure_blocking_hpm :
> - sht3x_cmd_measure_blocking_lpm;
> - data->wait_time = 0;
If update_interval is 0, those would presumably still be needed.
I don't know if the current code updating the interval is wrong
(that may well be), but removing this code entirely seems wrong.
> } else {
> - if (data->setup.high_precision) {
> + if (data->repeatability == high_repeatability) {
> data->command = sht3x_cmd_measure_nonblocking_hpm;
> data->wait_time = SHT3X_NONBLOCKING_WAIT_TIME_HPM;
> } else {
> @@ -595,7 +593,7 @@ static ssize_t update_interval_store(struct device *dev,
> }
>
> if (mode > 0) {
> - if (data->setup.high_precision)
> + if (data->repeatability == high_repeatability)
> command = periodic_measure_commands_hpm[mode - 1];
> else
> command = periodic_measure_commands_lpm[mode - 1];
> @@ -690,16 +688,12 @@ static int sht3x_probe(struct i2c_client *client)
> if (!data)
> return -ENOMEM;
>
> - data->setup.blocking_io = false;
> - data->setup.high_precision = true;
> + data->repeatability = high_repeatability;
> data->mode = 0;
> data->last_update = jiffies - msecs_to_jiffies(3000);
> data->client = client;
> crc8_populate_msb(sht3x_crc8_table, SHT3X_CRC8_POLYNOMIAL);
>
> - if (client->dev.platform_data)
> - data->setup = *(struct sht3x_platform_data *)dev->platform_data;
> -
> sht3x_select_command(data);
>
> mutex_init(&data->i2c_lock);
> diff --git a/include/linux/platform_data/sht3x.h b/include/linux/platform_data/sht3x.h
> deleted file mode 100644
> index 14680d2a9..000000000
> --- a/include/linux/platform_data/sht3x.h
> +++ /dev/null
> @@ -1,15 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0-or-later */
> -/*
> - * Copyright (C) 2016 Sensirion AG, Switzerland
> - * Author: David Frey <david.frey@sensirion.com>
> - * Author: Pascal Sachs <pascal.sachs@sensirion.com>
> - */
> -
> -#ifndef __SHT3X_H_
> -#define __SHT3X_H_
> -
> -struct sht3x_platform_data {
> - bool blocking_io;
> - bool high_precision;
> -};
> -#endif /* __SHT3X_H_ */
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] hwmon: (sht3x)remove sht3x_platform_data
2023-06-14 12:57 ` Guenter Roeck
@ 2023-06-14 15:02 ` JuenKit Yip
2023-06-14 19:15 ` Guenter Roeck
[not found] ` <01f88d7e-d461-8d73-6633-5b9060e42c37@hotmail.com>
1 sibling, 1 reply; 9+ messages in thread
From: JuenKit Yip @ 2023-06-14 15:02 UTC (permalink / raw)
To: Guenter Roeck; +Cc: linux-hwmon, linux-kernel, jdelvare
在 2023/6/14 20:57, Guenter Roeck 写道:
> On 6/13/23 23:24, JuenKit Yip wrote:
>> Since no in-tree driver supports it, the sht3x_platform_data was
>> removed.
>>
>> - "blocking_io" property and its related code have been removed, and
>> Single-Shot mode should be blocking in default.
>>
>> - "high-precision" property has been replaced to "repeatability" for
>> matching datasheet.
>>
>
> That needs to be three patches.
Patch 1: remove sht3x_platform_data and its header file
Patch 2: move "blocking_io" to struct sht3x_data
Patch 3: replace "high-precision" property to "repeatability"
Is it correct or I am misunderstand your statement?
Thanks for your instruction
>
>> Signed-off-by: JuenKit Yip <JuenKit_Yip@hotmail.com>
>> ---
>> Documentation/hwmon/sht3x.rst | 12 +++++------
>> drivers/hwmon/sht3x.c | 32 ++++++++++++-----------------
>> include/linux/platform_data/sht3x.h | 15 --------------
>> 3 files changed, 18 insertions(+), 41 deletions(-)
>> delete mode 100644 include/linux/platform_data/sht3x.h
>>
>> diff --git a/Documentation/hwmon/sht3x.rst
>> b/Documentation/hwmon/sht3x.rst
>> index 95a850d5b..2c87c8f58 100644
>> --- a/Documentation/hwmon/sht3x.rst
>> +++ b/Documentation/hwmon/sht3x.rst
>> @@ -28,28 +28,26 @@ The device communicates with the I2C protocol.
>> Sensors can have the I2C
>> addresses 0x44 or 0x45, depending on the wiring. See
>> Documentation/i2c/instantiating-devices.rst for methods to
>> instantiate the device.
>> -There are two options configurable by means of sht3x_platform_data:
>> +This driver supports block and non-block mode:
>> -1. blocking (pull the I2C clock line down while performing the
>> measurement) or
>> + blocking (pull the I2C clock line down while performing the
>> measurement) or
>> non-blocking mode. Blocking mode will guarantee the fastest
>> result but
>> the I2C bus will be busy during that time. By default,
>> non-blocking mode
>> is used. Make sure clock-stretching works properly on your
>> device if you
>> want to use blocking mode.
>> -2. high or low accuracy. High accuracy is used by default and using
>> it is
>> - strongly recommended.
>> The sht3x sensor supports a single shot mode as well as 5
>> periodic measure
>> modes, which can be controlled with the update_interval sysfs
>> interface.
>> The allowed update_interval in milliseconds are as follows:
>> - ===== ======= ====================
>> - 0 single shot mode
>> + ===== ======= ==========================
>> + 0 single shot mode(blocking)
>> 2000 0.5 Hz periodic measurement
>> 1000 1 Hz periodic measurement
>> 500 2 Hz periodic measurement
>> 250 4 Hz periodic measurement
>> 100 10 Hz periodic measurement
>> - ===== ======= ====================
>> + ===== ======= ==========================
>> In the periodic measure mode, the sensor automatically triggers a
>> measurement
>> with the configured update interval on the chip. When a temperature
>> or humidity
>> diff --git a/drivers/hwmon/sht3x.c b/drivers/hwmon/sht3x.c
>> index 8305e44d9..5bc0001b1 100644
>> --- a/drivers/hwmon/sht3x.c
>> +++ b/drivers/hwmon/sht3x.c
>> @@ -20,13 +20,12 @@
>> #include <linux/module.h>
>> #include <linux/slab.h>
>> #include <linux/jiffies.h>
>> -#include <linux/platform_data/sht3x.h>
>> -/* commands (high precision mode) */
>> +/* commands (high repeatability mode) */
>> static const unsigned char sht3x_cmd_measure_blocking_hpm[] = {
>> 0x2c, 0x06 };
>> static const unsigned char sht3x_cmd_measure_nonblocking_hpm[] = {
>> 0x24, 0x00 };
>> -/* commands (low power mode) */
>> +/* commands (low repeatability mode) */
>> static const unsigned char sht3x_cmd_measure_blocking_lpm[] = {
>> 0x2c, 0x10 };
>> static const unsigned char sht3x_cmd_measure_nonblocking_lpm[] = {
>> 0x24, 0x16 };
>> @@ -69,9 +68,14 @@ enum sht3x_limits {
>> limit_min_hyst,
>> };
>> +enum sht3x_repeatability {
>> + low_repeatability,
>> + high_repeatability,
>> +};
>> +
>> DECLARE_CRC8_TABLE(sht3x_crc8_table);
>> -/* periodic measure commands (high precision mode) */
>> +/* periodic measure commands (high repeatability mode) */
>> static const char periodic_measure_commands_hpm[][SHT3X_CMD_LENGTH]
>> = {
>> /* 0.5 measurements per second */
>> {0x20, 0x32},
>> @@ -85,7 +89,7 @@ static const char
>> periodic_measure_commands_hpm[][SHT3X_CMD_LENGTH] = {
>> {0x27, 0x37},
>> };
>> -/* periodic measure commands (low power mode) */
>> +/* periodic measure commands (low repeatability mode) */
>> static const char periodic_measure_commands_lpm[][SHT3X_CMD_LENGTH]
>> = {
>> /* 0.5 measurements per second */
>> {0x20, 0x2f},
>> @@ -132,12 +136,11 @@ struct sht3x_data {
>> struct mutex data_lock; /* lock for updating driver data */
>> u8 mode;
>> + enum sht3x_repeatability repeatability;
>> const unsigned char *command;
>> u32 wait_time; /* in us*/
>> unsigned long last_update; /* last update in periodic mode*/
>> - struct sht3x_platform_data setup;
>> -
>> /*
>> * cached values for temperature and humidity and limits
>> * the limits arrays have the following order:
>> @@ -441,13 +444,8 @@ static void sht3x_select_command(struct
>> sht3x_data *data)
>> if (data->mode > 0) {
>> data->command = sht3x_cmd_measure_periodic_mode;
>> data->wait_time = 0;
>> - } else if (data->setup.blocking_io) {
>> - data->command = data->setup.high_precision ?
>> - sht3x_cmd_measure_blocking_hpm :
>> - sht3x_cmd_measure_blocking_lpm;
>> - data->wait_time = 0;
>
> If update_interval is 0, those would presumably still be needed.
> I don't know if the current code updating the interval is wrong
> (that may well be), but removing this code entirely seems wrong.
update_interval "0" means Single-Shot mode and respectively data->command
should be in blocking mode.
Thanks for your correctness.
>
>> } else {
>> - if (data->setup.high_precision) {
>> + if (data->repeatability == high_repeatability) {
>> data->command = sht3x_cmd_measure_nonblocking_hpm;
>> data->wait_time = SHT3X_NONBLOCKING_WAIT_TIME_HPM;
>> } else {
>> @@ -595,7 +593,7 @@ static ssize_t update_interval_store(struct
>> device *dev,
>> }
>> if (mode > 0) {
>> - if (data->setup.high_precision)
>> + if (data->repeatability == high_repeatability)
>> command = periodic_measure_commands_hpm[mode - 1];
>> else
>> command = periodic_measure_commands_lpm[mode - 1];
>> @@ -690,16 +688,12 @@ static int sht3x_probe(struct i2c_client *client)
>> if (!data)
>> return -ENOMEM;
>> - data->setup.blocking_io = false;
>> - data->setup.high_precision = true;
>> + data->repeatability = high_repeatability;
>> data->mode = 0;
>> data->last_update = jiffies - msecs_to_jiffies(3000);
>> data->client = client;
>> crc8_populate_msb(sht3x_crc8_table, SHT3X_CRC8_POLYNOMIAL);
>> - if (client->dev.platform_data)
>> - data->setup = *(struct sht3x_platform_data
>> *)dev->platform_data;
>> -
>> sht3x_select_command(data);
>> mutex_init(&data->i2c_lock);
>> diff --git a/include/linux/platform_data/sht3x.h
>> b/include/linux/platform_data/sht3x.h
>> deleted file mode 100644
>> index 14680d2a9..000000000
>> --- a/include/linux/platform_data/sht3x.h
>> +++ /dev/null
>> @@ -1,15 +0,0 @@
>> -/* SPDX-License-Identifier: GPL-2.0-or-later */
>> -/*
>> - * Copyright (C) 2016 Sensirion AG, Switzerland
>> - * Author: David Frey <david.frey@sensirion.com>
>> - * Author: Pascal Sachs <pascal.sachs@sensirion.com>
>> - */
>> -
>> -#ifndef __SHT3X_H_
>> -#define __SHT3X_H_
>> -
>> -struct sht3x_platform_data {
>> - bool blocking_io;
>> - bool high_precision;
>> -};
>> -#endif /* __SHT3X_H_ */
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] hwmon: (sht3x)remove sht3x_platform_data
[not found] ` <01f88d7e-d461-8d73-6633-5b9060e42c37@hotmail.com>
@ 2023-06-14 15:12 ` JuenKit Yip
0 siblings, 0 replies; 9+ messages in thread
From: JuenKit Yip @ 2023-06-14 15:12 UTC (permalink / raw)
To: Guenter Roeck; +Cc: linux-hwmon, linux-kernel, jdelvare
在 2023/6/14 23:02, JuenKit Yip 写道:
>
> 在 2023/6/14 20:57, Guenter Roeck 写道:
>> On 6/13/23 23:24, JuenKit Yip wrote:
>>> Since no in-tree driver supports it, the sht3x_platform_data was
>>> removed.
>>>
>>> - "blocking_io" property and its related code have been removed, and
>>> Single-Shot mode should be blocking in default.
>>>
>>> - "high-precision" property has been replaced to "repeatability" for
>>> matching datasheet.
>>>
>>
>> That needs to be three patches.
>
> Patch 1: remove sht3x_platform_data and its header file
>
> Patch 2: move "blocking_io" to struct sht3x_data
>
> Patch 3: replace "high-precision" property to "repeatability"
>
> Is it correct or I am misunderstand your statement?
>
> Thanks for your instruction
>
>>
>>> Signed-off-by: JuenKit Yip <JuenKit_Yip@hotmail.com>
>>> ---
>>> Documentation/hwmon/sht3x.rst | 12 +++++------
>>> drivers/hwmon/sht3x.c | 32
>>> ++++++++++++-----------------
>>> include/linux/platform_data/sht3x.h | 15 --------------
>>> 3 files changed, 18 insertions(+), 41 deletions(-)
>>> delete mode 100644 include/linux/platform_data/sht3x.h
>>>
>>> diff --git a/Documentation/hwmon/sht3x.rst
>>> b/Documentation/hwmon/sht3x.rst
>>> index 95a850d5b..2c87c8f58 100644
>>> --- a/Documentation/hwmon/sht3x.rst
>>> +++ b/Documentation/hwmon/sht3x.rst
>>> @@ -28,28 +28,26 @@ The device communicates with the I2C protocol.
>>> Sensors can have the I2C
>>> addresses 0x44 or 0x45, depending on the wiring. See
>>> Documentation/i2c/instantiating-devices.rst for methods to
>>> instantiate the device.
>>> -There are two options configurable by means of sht3x_platform_data:
>>> +This driver supports block and non-block mode:
>>> -1. blocking (pull the I2C clock line down while performing the
>>> measurement) or
>>> + blocking (pull the I2C clock line down while performing the
>>> measurement) or
>>> non-blocking mode. Blocking mode will guarantee the fastest
>>> result but
>>> the I2C bus will be busy during that time. By default,
>>> non-blocking mode
>>> is used. Make sure clock-stretching works properly on your
>>> device if you
>>> want to use blocking mode.
>>> -2. high or low accuracy. High accuracy is used by default and using
>>> it is
>>> - strongly recommended.
>>> The sht3x sensor supports a single shot mode as well as 5
>>> periodic measure
>>> modes, which can be controlled with the update_interval sysfs
>>> interface.
>>> The allowed update_interval in milliseconds are as follows:
>>> - ===== ======= ====================
>>> - 0 single shot mode
>>> + ===== ======= ==========================
>>> + 0 single shot mode(blocking)
>>> 2000 0.5 Hz periodic measurement
>>> 1000 1 Hz periodic measurement
>>> 500 2 Hz periodic measurement
>>> 250 4 Hz periodic measurement
>>> 100 10 Hz periodic measurement
>>> - ===== ======= ====================
>>> + ===== ======= ==========================
>>> In the periodic measure mode, the sensor automatically triggers
>>> a measurement
>>> with the configured update interval on the chip. When a
>>> temperature or humidity
>>> diff --git a/drivers/hwmon/sht3x.c b/drivers/hwmon/sht3x.c
>>> index 8305e44d9..5bc0001b1 100644
>>> --- a/drivers/hwmon/sht3x.c
>>> +++ b/drivers/hwmon/sht3x.c
>>> @@ -20,13 +20,12 @@
>>> #include <linux/module.h>
>>> #include <linux/slab.h>
>>> #include <linux/jiffies.h>
>>> -#include <linux/platform_data/sht3x.h>
>>> -/* commands (high precision mode) */
>>> +/* commands (high repeatability mode) */
>>> static const unsigned char sht3x_cmd_measure_blocking_hpm[] = {
>>> 0x2c, 0x06 };
>>> static const unsigned char sht3x_cmd_measure_nonblocking_hpm[] = {
>>> 0x24, 0x00 };
>>> -/* commands (low power mode) */
>>> +/* commands (low repeatability mode) */
>>> static const unsigned char sht3x_cmd_measure_blocking_lpm[] = {
>>> 0x2c, 0x10 };
>>> static const unsigned char sht3x_cmd_measure_nonblocking_lpm[] = {
>>> 0x24, 0x16 };
>>> @@ -69,9 +68,14 @@ enum sht3x_limits {
>>> limit_min_hyst,
>>> };
>>> +enum sht3x_repeatability {
>>> + low_repeatability,
>>> + high_repeatability,
>>> +};
>>> +
>>> DECLARE_CRC8_TABLE(sht3x_crc8_table);
>>> -/* periodic measure commands (high precision mode) */
>>> +/* periodic measure commands (high repeatability mode) */
>>> static const char
>>> periodic_measure_commands_hpm[][SHT3X_CMD_LENGTH] = {
>>> /* 0.5 measurements per second */
>>> {0x20, 0x32},
>>> @@ -85,7 +89,7 @@ static const char
>>> periodic_measure_commands_hpm[][SHT3X_CMD_LENGTH] = {
>>> {0x27, 0x37},
>>> };
>>> -/* periodic measure commands (low power mode) */
>>> +/* periodic measure commands (low repeatability mode) */
>>> static const char
>>> periodic_measure_commands_lpm[][SHT3X_CMD_LENGTH] = {
>>> /* 0.5 measurements per second */
>>> {0x20, 0x2f},
>>> @@ -132,12 +136,11 @@ struct sht3x_data {
>>> struct mutex data_lock; /* lock for updating driver data */
>>> u8 mode;
>>> + enum sht3x_repeatability repeatability;
>>> const unsigned char *command;
>>> u32 wait_time; /* in us*/
>>> unsigned long last_update; /* last update in periodic mode*/
>>> - struct sht3x_platform_data setup;
>>> -
>>> /*
>>> * cached values for temperature and humidity and limits
>>> * the limits arrays have the following order:
>>> @@ -441,13 +444,8 @@ static void sht3x_select_command(struct
>>> sht3x_data *data)
>>> if (data->mode > 0) {
>>> data->command = sht3x_cmd_measure_periodic_mode;
>>> data->wait_time = 0;
>>> - } else if (data->setup.blocking_io) {
>>> - data->command = data->setup.high_precision ?
>>> - sht3x_cmd_measure_blocking_hpm :
>>> - sht3x_cmd_measure_blocking_lpm;
>>> - data->wait_time = 0;
>>
>> If update_interval is 0, those would presumably still be needed.
>> I don't know if the current code updating the interval is wrong
>> (that may well be), but removing this code entirely seems wrong.
>
> update_interval "0" means Single-Shot mode and respectively data->command
>
> should be in blocking mode or non-blocking mode about clock strench.
>
> Thanks for your correctness.
>>
>>> } else {
>>> - if (data->setup.high_precision) {
>>> + if (data->repeatability == high_repeatability) {
>>> data->command = sht3x_cmd_measure_nonblocking_hpm;
>>> data->wait_time = SHT3X_NONBLOCKING_WAIT_TIME_HPM;
>>> } else {
>>> @@ -595,7 +593,7 @@ static ssize_t update_interval_store(struct
>>> device *dev,
>>> }
>>> if (mode > 0) {
>>> - if (data->setup.high_precision)
>>> + if (data->repeatability == high_repeatability)
>>> command = periodic_measure_commands_hpm[mode - 1];
>>> else
>>> command = periodic_measure_commands_lpm[mode - 1];
>>> @@ -690,16 +688,12 @@ static int sht3x_probe(struct i2c_client *client)
>>> if (!data)
>>> return -ENOMEM;
>>> - data->setup.blocking_io = false;
>>> - data->setup.high_precision = true;
>>> + data->repeatability = high_repeatability;
>>> data->mode = 0;
>>> data->last_update = jiffies - msecs_to_jiffies(3000);
>>> data->client = client;
>>> crc8_populate_msb(sht3x_crc8_table, SHT3X_CRC8_POLYNOMIAL);
>>> - if (client->dev.platform_data)
>>> - data->setup = *(struct sht3x_platform_data
>>> *)dev->platform_data;
>>> -
>>> sht3x_select_command(data);
>>> mutex_init(&data->i2c_lock);
>>> diff --git a/include/linux/platform_data/sht3x.h
>>> b/include/linux/platform_data/sht3x.h
>>> deleted file mode 100644
>>> index 14680d2a9..000000000
>>> --- a/include/linux/platform_data/sht3x.h
>>> +++ /dev/null
>>> @@ -1,15 +0,0 @@
>>> -/* SPDX-License-Identifier: GPL-2.0-or-later */
>>> -/*
>>> - * Copyright (C) 2016 Sensirion AG, Switzerland
>>> - * Author: David Frey <david.frey@sensirion.com>
>>> - * Author: Pascal Sachs <pascal.sachs@sensirion.com>
>>> - */
>>> -
>>> -#ifndef __SHT3X_H_
>>> -#define __SHT3X_H_
>>> -
>>> -struct sht3x_platform_data {
>>> - bool blocking_io;
>>> - bool high_precision;
>>> -};
>>> -#endif /* __SHT3X_H_ */
>>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] hwmon: (sht3x)remove sht3x_platform_data
2023-06-14 6:24 [PATCH 1/3] hwmon: (sht3x)remove sht3x_platform_data JuenKit Yip
2023-06-14 10:05 ` kernel test robot
2023-06-14 12:57 ` Guenter Roeck
@ 2023-06-14 17:15 ` kernel test robot
2 siblings, 0 replies; 9+ messages in thread
From: kernel test robot @ 2023-06-14 17:15 UTC (permalink / raw)
To: JuenKit Yip, linux, jdelvare
Cc: oe-kbuild-all, linux-hwmon, linux-kernel, JuenKit Yip
Hi JuenKit,
kernel test robot noticed the following build warnings:
[auto build test WARNING on groeck-staging/hwmon-next]
[also build test WARNING on linus/master v6.4-rc6 next-20230614]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/JuenKit-Yip/hwmon-sht3x-add-medium-repeatability-support/20230614-143100
base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
patch link: https://lore.kernel.org/r/DB4PR10MB6261D79FE16EC2BBD5316B91925AA%40DB4PR10MB6261.EURPRD10.PROD.OUTLOOK.COM
patch subject: [PATCH 1/3] hwmon: (sht3x)remove sht3x_platform_data
config: i386-allyesconfig (https://download.01.org/0day-ci/archive/20230615/202306150115.i0k7ulfo-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build):
git remote add groeck-staging https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git
git fetch groeck-staging hwmon-next
git checkout groeck-staging/hwmon-next
b4 shazam https://lore.kernel.org/r/DB4PR10MB6261D79FE16EC2BBD5316B91925AA@DB4PR10MB6261.EURPRD10.PROD.OUTLOOK.COM
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 O=build_dir ARCH=i386 olddefconfig
make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/hwmon/
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306150115.i0k7ulfo-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/hwmon/sht3x.c:29:28: warning: 'sht3x_cmd_measure_blocking_lpm' defined but not used [-Wunused-const-variable=]
29 | static const unsigned char sht3x_cmd_measure_blocking_lpm[] = { 0x2c, 0x10 };
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/hwmon/sht3x.c:25:28: warning: 'sht3x_cmd_measure_blocking_hpm' defined but not used [-Wunused-const-variable=]
25 | static const unsigned char sht3x_cmd_measure_blocking_hpm[] = { 0x2c, 0x06 };
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +/sht3x_cmd_measure_blocking_lpm +29 drivers/hwmon/sht3x.c
7c84f7f80d6fcea David Frey 2016-06-02 23
cecbab8bdd40311 JuenKit Yip 2023-06-14 24 /* commands (high repeatability mode) */
7c84f7f80d6fcea David Frey 2016-06-02 @25 static const unsigned char sht3x_cmd_measure_blocking_hpm[] = { 0x2c, 0x06 };
7c84f7f80d6fcea David Frey 2016-06-02 26 static const unsigned char sht3x_cmd_measure_nonblocking_hpm[] = { 0x24, 0x00 };
7c84f7f80d6fcea David Frey 2016-06-02 27
cecbab8bdd40311 JuenKit Yip 2023-06-14 28 /* commands (low repeatability mode) */
7c84f7f80d6fcea David Frey 2016-06-02 @29 static const unsigned char sht3x_cmd_measure_blocking_lpm[] = { 0x2c, 0x10 };
7c84f7f80d6fcea David Frey 2016-06-02 30 static const unsigned char sht3x_cmd_measure_nonblocking_lpm[] = { 0x24, 0x16 };
7c84f7f80d6fcea David Frey 2016-06-02 31
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] hwmon: (sht3x)remove sht3x_platform_data
2023-06-14 15:02 ` JuenKit Yip
@ 2023-06-14 19:15 ` Guenter Roeck
2023-06-14 23:54 ` JuenKit Yip
0 siblings, 1 reply; 9+ messages in thread
From: Guenter Roeck @ 2023-06-14 19:15 UTC (permalink / raw)
To: JuenKit Yip; +Cc: linux-hwmon, linux-kernel, jdelvare
On 6/14/23 08:02, JuenKit Yip wrote:
>
> 在 2023/6/14 20:57, Guenter Roeck 写道:
>> On 6/13/23 23:24, JuenKit Yip wrote:
>>> Since no in-tree driver supports it, the sht3x_platform_data was
>>> removed.
>>>
>>> - "blocking_io" property and its related code have been removed, and
>>> Single-Shot mode should be blocking in default.
>>>
>>> - "high-precision" property has been replaced to "repeatability" for
>>> matching datasheet.
>>>
>>
>> That needs to be three patches.
>
> Patch 1: remove sht3x_platform_data and its header file
>
> Patch 2: move "blocking_io" to struct sht3x_data
>
Essentially merge it with update_interval==0 since (if I understand
correctly) blocking mode and update_interval==0 will be equivalent.
With that in mind, a separate "blocking_io" variable should no
longer be needed.
> Patch 3: replace "high-precision" property to "repeatability"
precision -> reliability would apply to both high and low
precision. I think "low repeatability" is currently called
"low power mode", so you'll want to update that as well.
I also see "high accuracy" and "low accuracy" use in the
documentation, but I see you removed that already below.
>
> Is it correct or I am misunderstand your statement?
>
Yes, that is what I meant.
Thanks,
Guenter
> Thanks for your instruction
>
>>
>>> Signed-off-by: JuenKit Yip <JuenKit_Yip@hotmail.com>
>>> ---
>>> Documentation/hwmon/sht3x.rst | 12 +++++------
>>> drivers/hwmon/sht3x.c | 32 ++++++++++++-----------------
>>> include/linux/platform_data/sht3x.h | 15 --------------
>>> 3 files changed, 18 insertions(+), 41 deletions(-)
>>> delete mode 100644 include/linux/platform_data/sht3x.h
>>>
>>> diff --git a/Documentation/hwmon/sht3x.rst b/Documentation/hwmon/sht3x.rst
>>> index 95a850d5b..2c87c8f58 100644
>>> --- a/Documentation/hwmon/sht3x.rst
>>> +++ b/Documentation/hwmon/sht3x.rst
>>> @@ -28,28 +28,26 @@ The device communicates with the I2C protocol. Sensors can have the I2C
>>> addresses 0x44 or 0x45, depending on the wiring. See
>>> Documentation/i2c/instantiating-devices.rst for methods to instantiate the device.
>>> -There are two options configurable by means of sht3x_platform_data:
>>> +This driver supports block and non-block mode:
>>> -1. blocking (pull the I2C clock line down while performing the measurement) or
>>> + blocking (pull the I2C clock line down while performing the measurement) or
>>> non-blocking mode. Blocking mode will guarantee the fastest result but
>>> the I2C bus will be busy during that time. By default, non-blocking mode
>>> is used. Make sure clock-stretching works properly on your device if you
>>> want to use blocking mode.
>>> -2. high or low accuracy. High accuracy is used by default and using it is
>>> - strongly recommended.
>>> The sht3x sensor supports a single shot mode as well as 5 periodic measure
>>> modes, which can be controlled with the update_interval sysfs interface.
>>> The allowed update_interval in milliseconds are as follows:
>>> - ===== ======= ====================
>>> - 0 single shot mode
>>> + ===== ======= ==========================
>>> + 0 single shot mode(blocking)
>>> 2000 0.5 Hz periodic measurement
>>> 1000 1 Hz periodic measurement
>>> 500 2 Hz periodic measurement
>>> 250 4 Hz periodic measurement
>>> 100 10 Hz periodic measurement
>>> - ===== ======= ====================
>>> + ===== ======= ==========================
>>> In the periodic measure mode, the sensor automatically triggers a measurement
>>> with the configured update interval on the chip. When a temperature or humidity
>>> diff --git a/drivers/hwmon/sht3x.c b/drivers/hwmon/sht3x.c
>>> index 8305e44d9..5bc0001b1 100644
>>> --- a/drivers/hwmon/sht3x.c
>>> +++ b/drivers/hwmon/sht3x.c
>>> @@ -20,13 +20,12 @@
>>> #include <linux/module.h>
>>> #include <linux/slab.h>
>>> #include <linux/jiffies.h>
>>> -#include <linux/platform_data/sht3x.h>
>>> -/* commands (high precision mode) */
>>> +/* commands (high repeatability mode) */
>>> static const unsigned char sht3x_cmd_measure_blocking_hpm[] = { 0x2c, 0x06 };
>>> static const unsigned char sht3x_cmd_measure_nonblocking_hpm[] = { 0x24, 0x00 };
>>> -/* commands (low power mode) */
>>> +/* commands (low repeatability mode) */
>>> static const unsigned char sht3x_cmd_measure_blocking_lpm[] = { 0x2c, 0x10 };
>>> static const unsigned char sht3x_cmd_measure_nonblocking_lpm[] = { 0x24, 0x16 };
>>> @@ -69,9 +68,14 @@ enum sht3x_limits {
>>> limit_min_hyst,
>>> };
>>> +enum sht3x_repeatability {
>>> + low_repeatability,
>>> + high_repeatability,
>>> +};
>>> +
>>> DECLARE_CRC8_TABLE(sht3x_crc8_table);
>>> -/* periodic measure commands (high precision mode) */
>>> +/* periodic measure commands (high repeatability mode) */
>>> static const char periodic_measure_commands_hpm[][SHT3X_CMD_LENGTH] = {
>>> /* 0.5 measurements per second */
>>> {0x20, 0x32},
>>> @@ -85,7 +89,7 @@ static const char periodic_measure_commands_hpm[][SHT3X_CMD_LENGTH] = {
>>> {0x27, 0x37},
>>> };
>>> -/* periodic measure commands (low power mode) */
>>> +/* periodic measure commands (low repeatability mode) */
>>> static const char periodic_measure_commands_lpm[][SHT3X_CMD_LENGTH] = {
>>> /* 0.5 measurements per second */
>>> {0x20, 0x2f},
>>> @@ -132,12 +136,11 @@ struct sht3x_data {
>>> struct mutex data_lock; /* lock for updating driver data */
>>> u8 mode;
>>> + enum sht3x_repeatability repeatability;
>>> const unsigned char *command;
>>> u32 wait_time; /* in us*/
>>> unsigned long last_update; /* last update in periodic mode*/
>>> - struct sht3x_platform_data setup;
>>> -
>>> /*
>>> * cached values for temperature and humidity and limits
>>> * the limits arrays have the following order:
>>> @@ -441,13 +444,8 @@ static void sht3x_select_command(struct sht3x_data *data)
>>> if (data->mode > 0) {
>>> data->command = sht3x_cmd_measure_periodic_mode;
>>> data->wait_time = 0;
>>> - } else if (data->setup.blocking_io) {
>>> - data->command = data->setup.high_precision ?
>>> - sht3x_cmd_measure_blocking_hpm :
>>> - sht3x_cmd_measure_blocking_lpm;
>>> - data->wait_time = 0;
>>
>> If update_interval is 0, those would presumably still be needed.
>> I don't know if the current code updating the interval is wrong
>> (that may well be), but removing this code entirely seems wrong.
>
> update_interval "0" means Single-Shot mode and respectively data->command
>
> should be in blocking mode.
>
> Thanks for your correctness.
>
>>
>>> } else {
>>> - if (data->setup.high_precision) {
>>> + if (data->repeatability == high_repeatability) {
>>> data->command = sht3x_cmd_measure_nonblocking_hpm;
>>> data->wait_time = SHT3X_NONBLOCKING_WAIT_TIME_HPM;
>>> } else {
>>> @@ -595,7 +593,7 @@ static ssize_t update_interval_store(struct device *dev,
>>> }
>>> if (mode > 0) {
>>> - if (data->setup.high_precision)
>>> + if (data->repeatability == high_repeatability)
>>> command = periodic_measure_commands_hpm[mode - 1];
>>> else
>>> command = periodic_measure_commands_lpm[mode - 1];
>>> @@ -690,16 +688,12 @@ static int sht3x_probe(struct i2c_client *client)
>>> if (!data)
>>> return -ENOMEM;
>>> - data->setup.blocking_io = false;
>>> - data->setup.high_precision = true;
>>> + data->repeatability = high_repeatability;
>>> data->mode = 0;
>>> data->last_update = jiffies - msecs_to_jiffies(3000);
>>> data->client = client;
>>> crc8_populate_msb(sht3x_crc8_table, SHT3X_CRC8_POLYNOMIAL);
>>> - if (client->dev.platform_data)
>>> - data->setup = *(struct sht3x_platform_data *)dev->platform_data;
>>> -
>>> sht3x_select_command(data);
>>> mutex_init(&data->i2c_lock);
>>> diff --git a/include/linux/platform_data/sht3x.h b/include/linux/platform_data/sht3x.h
>>> deleted file mode 100644
>>> index 14680d2a9..000000000
>>> --- a/include/linux/platform_data/sht3x.h
>>> +++ /dev/null
>>> @@ -1,15 +0,0 @@
>>> -/* SPDX-License-Identifier: GPL-2.0-or-later */
>>> -/*
>>> - * Copyright (C) 2016 Sensirion AG, Switzerland
>>> - * Author: David Frey <david.frey@sensirion.com>
>>> - * Author: Pascal Sachs <pascal.sachs@sensirion.com>
>>> - */
>>> -
>>> -#ifndef __SHT3X_H_
>>> -#define __SHT3X_H_
>>> -
>>> -struct sht3x_platform_data {
>>> - bool blocking_io;
>>> - bool high_precision;
>>> -};
>>> -#endif /* __SHT3X_H_ */
>>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] hwmon: (sht3x)remove sht3x_platform_data
2023-06-14 19:15 ` Guenter Roeck
@ 2023-06-14 23:54 ` JuenKit Yip
2023-06-15 14:56 ` Guenter Roeck
0 siblings, 1 reply; 9+ messages in thread
From: JuenKit Yip @ 2023-06-14 23:54 UTC (permalink / raw)
To: Guenter Roeck; +Cc: linux-hwmon, linux-kernel, jdelvare
在 2023/6/15 3:15, Guenter Roeck 写道:
> On 6/14/23 08:02, JuenKit Yip wrote:
>>
>> 在 2023/6/14 20:57, Guenter Roeck 写道:
>>> On 6/13/23 23:24, JuenKit Yip wrote:
>>>> Since no in-tree driver supports it, the sht3x_platform_data was
>>>> removed.
>>>>
>>>> - "blocking_io" property and its related code have been removed, and
>>>> Single-Shot mode should be blocking in default.
>>>>
>>>> - "high-precision" property has been replaced to "repeatability" for
>>>> matching datasheet.
>>>>
>>>
>>> That needs to be three patches.
>>
>> Patch 1: remove sht3x_platform_data and its header file
>>
>> Patch 2: move "blocking_io" to struct sht3x_data
>>
> Essentially merge it with update_interval==0 since (if I understand
> correctly) blocking mode and update_interval==0 will be equivalent.
> With that in mind, a separate "blocking_io" variable should no
> longer be needed.
>
I reviewed the datasheet again, update_interval == 0 means Single-Shot
mode which owns blocking(clock strench) and non-blocking(non-clock strench)
options. If master supports clock-strench( I don't know how to detect it),
the property may be reserved.
>> Patch 3: replace "high-precision" property to "repeatability"
>
> precision -> reliability would apply to both high and low
> precision. I think "low repeatability" is currently called
> "low power mode", so you'll want to update that as well.
> I also see "high accuracy" and "low accuracy" use in the
> documentation, but I see you removed that already below.
>
>>
>> Is it correct or I am misunderstand your statement?
>>
>
> Yes, that is what I meant.
>
> Thanks,
> Guenter
>
>> Thanks for your instruction
>>
>>>
>>>> Signed-off-by: JuenKit Yip <JuenKit_Yip@hotmail.com>
>>>> ---
>>>> Documentation/hwmon/sht3x.rst | 12 +++++------
>>>> drivers/hwmon/sht3x.c | 32
>>>> ++++++++++++-----------------
>>>> include/linux/platform_data/sht3x.h | 15 --------------
>>>> 3 files changed, 18 insertions(+), 41 deletions(-)
>>>> delete mode 100644 include/linux/platform_data/sht3x.h
>>>>
>>>> diff --git a/Documentation/hwmon/sht3x.rst
>>>> b/Documentation/hwmon/sht3x.rst
>>>> index 95a850d5b..2c87c8f58 100644
>>>> --- a/Documentation/hwmon/sht3x.rst
>>>> +++ b/Documentation/hwmon/sht3x.rst
>>>> @@ -28,28 +28,26 @@ The device communicates with the I2C protocol.
>>>> Sensors can have the I2C
>>>> addresses 0x44 or 0x45, depending on the wiring. See
>>>> Documentation/i2c/instantiating-devices.rst for methods to
>>>> instantiate the device.
>>>> -There are two options configurable by means of sht3x_platform_data:
>>>> +This driver supports block and non-block mode:
>>>> -1. blocking (pull the I2C clock line down while performing the
>>>> measurement) or
>>>> + blocking (pull the I2C clock line down while performing the
>>>> measurement) or
>>>> non-blocking mode. Blocking mode will guarantee the fastest
>>>> result but
>>>> the I2C bus will be busy during that time. By default,
>>>> non-blocking mode
>>>> is used. Make sure clock-stretching works properly on your
>>>> device if you
>>>> want to use blocking mode.
>>>> -2. high or low accuracy. High accuracy is used by default and
>>>> using it is
>>>> - strongly recommended.
>>>> The sht3x sensor supports a single shot mode as well as 5
>>>> periodic measure
>>>> modes, which can be controlled with the update_interval sysfs
>>>> interface.
>>>> The allowed update_interval in milliseconds are as follows:
>>>> - ===== ======= ====================
>>>> - 0 single shot mode
>>>> + ===== ======= ==========================
>>>> + 0 single shot mode(blocking)
>>>> 2000 0.5 Hz periodic measurement
>>>> 1000 1 Hz periodic measurement
>>>> 500 2 Hz periodic measurement
>>>> 250 4 Hz periodic measurement
>>>> 100 10 Hz periodic measurement
>>>> - ===== ======= ====================
>>>> + ===== ======= ==========================
>>>> In the periodic measure mode, the sensor automatically triggers
>>>> a measurement
>>>> with the configured update interval on the chip. When a
>>>> temperature or humidity
>>>> diff --git a/drivers/hwmon/sht3x.c b/drivers/hwmon/sht3x.c
>>>> index 8305e44d9..5bc0001b1 100644
>>>> --- a/drivers/hwmon/sht3x.c
>>>> +++ b/drivers/hwmon/sht3x.c
>>>> @@ -20,13 +20,12 @@
>>>> #include <linux/module.h>
>>>> #include <linux/slab.h>
>>>> #include <linux/jiffies.h>
>>>> -#include <linux/platform_data/sht3x.h>
>>>> -/* commands (high precision mode) */
>>>> +/* commands (high repeatability mode) */
>>>> static const unsigned char sht3x_cmd_measure_blocking_hpm[] = {
>>>> 0x2c, 0x06 };
>>>> static const unsigned char sht3x_cmd_measure_nonblocking_hpm[] =
>>>> { 0x24, 0x00 };
>>>> -/* commands (low power mode) */
>>>> +/* commands (low repeatability mode) */
>>>> static const unsigned char sht3x_cmd_measure_blocking_lpm[] = {
>>>> 0x2c, 0x10 };
>>>> static const unsigned char sht3x_cmd_measure_nonblocking_lpm[] =
>>>> { 0x24, 0x16 };
>>>> @@ -69,9 +68,14 @@ enum sht3x_limits {
>>>> limit_min_hyst,
>>>> };
>>>> +enum sht3x_repeatability {
>>>> + low_repeatability,
>>>> + high_repeatability,
>>>> +};
>>>> +
>>>> DECLARE_CRC8_TABLE(sht3x_crc8_table);
>>>> -/* periodic measure commands (high precision mode) */
>>>> +/* periodic measure commands (high repeatability mode) */
>>>> static const char
>>>> periodic_measure_commands_hpm[][SHT3X_CMD_LENGTH] = {
>>>> /* 0.5 measurements per second */
>>>> {0x20, 0x32},
>>>> @@ -85,7 +89,7 @@ static const char
>>>> periodic_measure_commands_hpm[][SHT3X_CMD_LENGTH] = {
>>>> {0x27, 0x37},
>>>> };
>>>> -/* periodic measure commands (low power mode) */
>>>> +/* periodic measure commands (low repeatability mode) */
>>>> static const char
>>>> periodic_measure_commands_lpm[][SHT3X_CMD_LENGTH] = {
>>>> /* 0.5 measurements per second */
>>>> {0x20, 0x2f},
>>>> @@ -132,12 +136,11 @@ struct sht3x_data {
>>>> struct mutex data_lock; /* lock for updating driver data */
>>>> u8 mode;
>>>> + enum sht3x_repeatability repeatability;
>>>> const unsigned char *command;
>>>> u32 wait_time; /* in us*/
>>>> unsigned long last_update; /* last update in periodic mode*/
>>>> - struct sht3x_platform_data setup;
>>>> -
>>>> /*
>>>> * cached values for temperature and humidity and limits
>>>> * the limits arrays have the following order:
>>>> @@ -441,13 +444,8 @@ static void sht3x_select_command(struct
>>>> sht3x_data *data)
>>>> if (data->mode > 0) {
>>>> data->command = sht3x_cmd_measure_periodic_mode;
>>>> data->wait_time = 0;
>>>> - } else if (data->setup.blocking_io) {
>>>> - data->command = data->setup.high_precision ?
>>>> - sht3x_cmd_measure_blocking_hpm :
>>>> - sht3x_cmd_measure_blocking_lpm;
>>>> - data->wait_time = 0;
>>>
>>> If update_interval is 0, those would presumably still be needed.
>>> I don't know if the current code updating the interval is wrong
>>> (that may well be), but removing this code entirely seems wrong.
>>
>> update_interval "0" means Single-Shot mode and respectively
>> data->command
>>
>> should be in blocking mode.
>>
>> Thanks for your correctness.
>>
>>>
>>>> } else {
>>>> - if (data->setup.high_precision) {
>>>> + if (data->repeatability == high_repeatability) {
>>>> data->command = sht3x_cmd_measure_nonblocking_hpm;
>>>> data->wait_time = SHT3X_NONBLOCKING_WAIT_TIME_HPM;
>>>> } else {
>>>> @@ -595,7 +593,7 @@ static ssize_t update_interval_store(struct
>>>> device *dev,
>>>> }
>>>> if (mode > 0) {
>>>> - if (data->setup.high_precision)
>>>> + if (data->repeatability == high_repeatability)
>>>> command = periodic_measure_commands_hpm[mode - 1];
>>>> else
>>>> command = periodic_measure_commands_lpm[mode - 1];
>>>> @@ -690,16 +688,12 @@ static int sht3x_probe(struct i2c_client
>>>> *client)
>>>> if (!data)
>>>> return -ENOMEM;
>>>> - data->setup.blocking_io = false;
>>>> - data->setup.high_precision = true;
>>>> + data->repeatability = high_repeatability;
>>>> data->mode = 0;
>>>> data->last_update = jiffies - msecs_to_jiffies(3000);
>>>> data->client = client;
>>>> crc8_populate_msb(sht3x_crc8_table, SHT3X_CRC8_POLYNOMIAL);
>>>> - if (client->dev.platform_data)
>>>> - data->setup = *(struct sht3x_platform_data
>>>> *)dev->platform_data;
>>>> -
>>>> sht3x_select_command(data);
>>>> mutex_init(&data->i2c_lock);
>>>> diff --git a/include/linux/platform_data/sht3x.h
>>>> b/include/linux/platform_data/sht3x.h
>>>> deleted file mode 100644
>>>> index 14680d2a9..000000000
>>>> --- a/include/linux/platform_data/sht3x.h
>>>> +++ /dev/null
>>>> @@ -1,15 +0,0 @@
>>>> -/* SPDX-License-Identifier: GPL-2.0-or-later */
>>>> -/*
>>>> - * Copyright (C) 2016 Sensirion AG, Switzerland
>>>> - * Author: David Frey <david.frey@sensirion.com>
>>>> - * Author: Pascal Sachs <pascal.sachs@sensirion.com>
>>>> - */
>>>> -
>>>> -#ifndef __SHT3X_H_
>>>> -#define __SHT3X_H_
>>>> -
>>>> -struct sht3x_platform_data {
>>>> - bool blocking_io;
>>>> - bool high_precision;
>>>> -};
>>>> -#endif /* __SHT3X_H_ */
>>>
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/3] hwmon: (sht3x)remove sht3x_platform_data
2023-06-14 23:54 ` JuenKit Yip
@ 2023-06-15 14:56 ` Guenter Roeck
0 siblings, 0 replies; 9+ messages in thread
From: Guenter Roeck @ 2023-06-15 14:56 UTC (permalink / raw)
To: JuenKit Yip; +Cc: linux-hwmon, linux-kernel, jdelvare
On 6/14/23 16:54, JuenKit Yip wrote:
>
> 在 2023/6/15 3:15, Guenter Roeck 写道:
>> On 6/14/23 08:02, JuenKit Yip wrote:
>>>
>>> 在 2023/6/14 20:57, Guenter Roeck 写道:
>>>> On 6/13/23 23:24, JuenKit Yip wrote:
>>>>> Since no in-tree driver supports it, the sht3x_platform_data was
>>>>> removed.
>>>>>
>>>>> - "blocking_io" property and its related code have been removed, and
>>>>> Single-Shot mode should be blocking in default.
>>>>>
>>>>> - "high-precision" property has been replaced to "repeatability" for
>>>>> matching datasheet.
>>>>>
>>>>
>>>> That needs to be three patches.
>>>
>>> Patch 1: remove sht3x_platform_data and its header file
>>>
>>> Patch 2: move "blocking_io" to struct sht3x_data
>>>
>> Essentially merge it with update_interval==0 since (if I understand
>> correctly) blocking mode and update_interval==0 will be equivalent.
>> With that in mind, a separate "blocking_io" variable should no
>> longer be needed.
>>
> I reviewed the datasheet again, update_interval == 0 means Single-Shot
>
> mode which owns blocking(clock strench) and non-blocking(non-clock strench)
>
> options. If master supports clock-strench( I don't know how to detect it),
>
> the property may be reserved.
>
I see.
In practice, blocking mode was never really used, at least not in the
upstream kernel, since platform data was not set from any in-kernel
code. Given that it is pretty much untested, I would suggest to always
use non-blocking mode. This is only relevant if the chip is in
single-shot mode, so worst case the user would have to wait a bit longer
for results in that mode. I find that acceptable over the risk involved
when trying to use/support blocking mode.
Guenter
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-06-15 14:57 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-06-14 6:24 [PATCH 1/3] hwmon: (sht3x)remove sht3x_platform_data JuenKit Yip
2023-06-14 10:05 ` kernel test robot
2023-06-14 12:57 ` Guenter Roeck
2023-06-14 15:02 ` JuenKit Yip
2023-06-14 19:15 ` Guenter Roeck
2023-06-14 23:54 ` JuenKit Yip
2023-06-15 14:56 ` Guenter Roeck
[not found] ` <01f88d7e-d461-8d73-6633-5b9060e42c37@hotmail.com>
2023-06-14 15:12 ` JuenKit Yip
2023-06-14 17:15 ` kernel test robot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox