Message-ID: <45c8771d-47c1-c007-1cd5-97eebedef147@roeck-us.net>
Date:   Wed, 14 Jun 2023 12:15:28 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     JuenKit Yip <JuenKit_Yip@...mail.com>
Cc:     linux-hwmon@...r.kernel.org, linux-kernel@...r.kernel.org,
        jdelvare@...e.com
Subject: Re: [PATCH 1/3] hwmon: (sht3x)remove sht3x_platform_data

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@...mail.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@...sirion.com>
>>> - * Author: Pascal Sachs <pascal.sachs@...sirion.com>
>>> - */
>>> -
>>> -#ifndef __SHT3X_H_
>>> -#define __SHT3X_H_
>>> -
>>> -struct sht3x_platform_data {
>>> -    bool blocking_io;
>>> -    bool high_precision;
>>> -};
>>> -#endif /* __SHT3X_H_ */
>>

Powered by blists - more mailing lists