Re: [RFC] sht3x code modifcation

[Guenter Roeck]

On 6/12/23 02:58, JuenKit Yip wrote:
> The original code only support two mode: high-precision mode(high repeatability and
> 10Hz periodic measurement) and low-power mode(low repeatability and 0.5Hz measurement),
> but in fact this sensor support 5 periodic measurement duration(and single shot) and
> 3 repeatability which are not fully implemented.
> High-precision mode was defined manually so that I think that we should reserve the
> right to user to choose which one repeatability and periodic measurement is the best.
> I just put the patch for reference and hope you could give any comments.
> Medium-repeatability was not added into code and I hope put it later.
>
> If you have any other further question, kindly contact to me.
> Thanks
>
> Juen Kit Yip
>
> Signed-off-by: JuenKit Yip <JuenKit_Yip@xxxxxxxxxxx>

So this patch replaces the term "high precision" with "high repeatability",
"low power" with "low repeatability", and it removes the ability to select
high vs. low precision / repeatability from platform data.

I would accept a patch removing platform data entirely since there is
no in-tree driver supporting it. The ability to select precision
(or reliability) should still be supported, possibly with a non-standard
sysfs attribute. You might consider sending a series of patches.

1) Remove platform data support
2) (optional) Convert driver to with_info API
3) Replace "precision" with "repeatability" to match datasheet
   terminology
4) Implement a non-standard sysfs attribute such as "repeatability"
   to support selecting high/medium/low repeatability.
5) (optional) Implement devicetree support.

Guenter

> ---
>   Documentation/hwmon/sht3x.rst       |  6 ++----
>   drivers/hwmon/sht3x.c               | 27 +++++++++++++++++----------
>   include/linux/platform_data/sht3x.h |  1 -
>   3 files changed, 19 insertions(+), 15 deletions(-)
>
> diff --git a/Documentation/hwmon/sht3x.rst b/Documentation/hwmon/sht3x.rst
> index 95a850d5b..c6b7a1aa5 100644
> --- a/Documentation/hwmon/sht3x.rst
> +++ b/Documentation/hwmon/sht3x.rst
> @@ -28,15 +28,13 @@ 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:
> +There is only one option configurable by means of sht3x_platform_data:
>
> -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.
> diff --git a/drivers/hwmon/sht3x.c b/drivers/hwmon/sht3x.c
> index 8305e44d9..6065312ae 100644
> --- a/drivers/hwmon/sht3x.c
> +++ b/drivers/hwmon/sht3x.c
> @@ -22,11 +22,11 @@
>   #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 +69,14 @@ enum sht3x_limits {
>       limit_min_hyst,
>   };
>
> +enum sht3x_repeatability {
> +    high_repeatability,
> +    low_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 +90,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,6 +137,7 @@ 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*/
> @@ -442,12 +448,13 @@ static void sht3x_select_command(struct sht3x_data *data)
>           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;
> +        if(data->repeatability == high_repeatability)
> +            data->command = sht3x_cmd_measure_blocking_hpm;
> +        else if(data->repeatability == low_repeatability)
> +            data->command = 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 +602,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];
> @@ -691,7 +698,7 @@ static int sht3x_probe(struct i2c_client *client)
>           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;
> diff --git a/include/linux/platform_data/sht3x.h b/include/linux/platform_data/sht3x.h
> index 14680d2a9..626c1404a 100644
> --- a/include/linux/platform_data/sht3x.h
> +++ b/include/linux/platform_data/sht3x.h
> @@ -10,6 +10,5 @@
>
>   struct sht3x_platform_data {
>       bool blocking_io;
> -    bool high_precision;
>   };
>   #endif /* __SHT3X_H_ */