Re: [PATCH 1/3] hwmon: (sht3x)remove sht3x_platform_data

[JuenKit Yip <JuenKit_Yip@xxxxxxxxxxx>]

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