Message-ID: <DB4PR10MB6261C86DEE5F4008B20C5033925AA@DB4PR10MB6261.EURPRD10.PROD.OUTLOOK.COM>
Date:   Thu, 15 Jun 2023 07:54:29 +0800
From:   JuenKit Yip <JuenKit_Yip@...mail.com>
To:     Guenter Roeck <linux@...ck-us.net>
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


在 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@...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