> -----Original Message-----
> From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck
> Sent: Saturday, September 16, 2023 2:26 AM
> To: Matyas, Daniel <Daniel.Matyas@xxxxxxxxxx>
> Cc: Jean Delvare <jdelvare@xxxxxxxx>; Rob Herring
> <robh+dt@xxxxxxxxxx>; Krzysztof Kozlowski
> <krzysztof.kozlowski+dt@xxxxxxxxxx>; Conor Dooley
> <conor+dt@xxxxxxxxxx>; Jonathan Corbet <corbet@xxxxxxx>; linux-
> hwmon@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; linux-doc@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v3 1/5] hwmon: max31827: Make code cleaner
>
> [External]
>
> On 9/14/23 00:59, Daniel Matyas wrote:
> > Now the wait time for one-shot is 140ms, instead of the old 141
> > (removed the 1ms error).
> >
>
> It was explicitly documented that the wait time was 140 + 1 milli-seconds,
> presumably to be sure that the conversion is really complete.
>
> Why was this an error ? It was _documented_ that way.
>
> Guenter
>
Well... actually I developed the driver initially and I wrote the documentation, so I know. I decided to remove the error milli-second, because I realized, it isn't really needed. There is no reference about it in the documentation of the chip, and frankly, I didn’t actually encounter any error which would need the 1 milli-second.
This way, the wait time is more exact and the correspondence with the chip documentation becomes quite straightforward.
Daniel
> > Used enums and while loops to replace switch for selecting and getting
> > update interval from conversion rate bits.
> >
> > Divided the write_alarm_val function into 2 functions. The new
> > function is more generic: it can be used not only for alarm writes,
> > but for any kind of writes which require the device to be in shutdown
> mode.
> >
> > Signed-off-by: Daniel Matyas <daniel.matyas@xxxxxxxxxx>
> > ---
> >
> > v2 -> v3: No change.
> >
> > v2: Added patch.
> >
> > Documentation/hwmon/max31827.rst | 4 +-
> > drivers/hwmon/max31827.c | 127 ++++++++++++++-----------------
> > 2 files changed, 58 insertions(+), 73 deletions(-)
> >
> > diff --git a/Documentation/hwmon/max31827.rst
> > b/Documentation/hwmon/max31827.rst
> > index b0971d05b8a4..9a1055a007cf 100644
> > --- a/Documentation/hwmon/max31827.rst
> > +++ b/Documentation/hwmon/max31827.rst
> > @@ -73,8 +73,8 @@ the conversion frequency to 1 conv/s. The
> conversion time varies depending on
> > the resolution. The conversion time doubles with every bit of increased
> > resolution. For 10 bit resolution 35ms are needed, while for 12 bit
> resolution
> > (default) 140ms. When chip is in shutdown mode and a read operation
> > is -requested, one-shot is triggered, the device waits for 140
> > (conversion time) + 1
> > -(error) ms, and only after that is the temperature value register read.
> > +requested, one-shot is triggered, the device waits for 140
> > +(conversion time) ms, and only after that is the temperature value
> register read.
> >
> > The LSB of the temperature values is 0.0625 degrees Celsius, but the
> values of
> > the temperatures are displayed in milli-degrees. This means, that
> > some data is diff --git a/drivers/hwmon/max31827.c
> > b/drivers/hwmon/max31827.c index 602f4e4f81ff..f05762219995
> 100644
> > --- a/drivers/hwmon/max31827.c
> > +++ b/drivers/hwmon/max31827.c
> > @@ -25,20 +25,32 @@
> > #define MAX31827_CONFIGURATION_U_TEMP_STAT_MASK BIT(14)
> > #define MAX31827_CONFIGURATION_O_TEMP_STAT_MASK BIT(15)
> >
> > -#define MAX31827_12_BIT_CNV_TIME 141
> > -
> > -#define MAX31827_CNV_1_DIV_64_HZ 0x1
> > -#define MAX31827_CNV_1_DIV_32_HZ 0x2
> > -#define MAX31827_CNV_1_DIV_16_HZ 0x3
> > -#define MAX31827_CNV_1_DIV_4_HZ 0x4
> > -#define MAX31827_CNV_1_HZ 0x5
> > -#define MAX31827_CNV_4_HZ 0x6
> > -#define MAX31827_CNV_8_HZ 0x7
> > +#define MAX31827_12_BIT_CNV_TIME 140
> >
> > #define MAX31827_16_BIT_TO_M_DGR(x) (sign_extend32(x, 15) *
> 1000 / 16)
> > #define MAX31827_M_DGR_TO_16_BIT(x) (((x) << 4) / 1000)
> > #define MAX31827_DEVICE_ENABLE(x) ((x) ? 0xA : 0x0)
> >
> > +enum max31827_cnv {
> > + MAX31827_CNV_1_DIV_64_HZ = 1,
> > + MAX31827_CNV_1_DIV_32_HZ,
> > + MAX31827_CNV_1_DIV_16_HZ,
> > + MAX31827_CNV_1_DIV_4_HZ,
> > + MAX31827_CNV_1_HZ,
> > + MAX31827_CNV_4_HZ,
> > + MAX31827_CNV_8_HZ,
> > +};
> > +
> > +static const u16 max31827_conversions[] = {
> > + [MAX31827_CNV_1_DIV_64_HZ] = 64000,
> > + [MAX31827_CNV_1_DIV_32_HZ] = 32000,
> > + [MAX31827_CNV_1_DIV_16_HZ] = 16000,
> > + [MAX31827_CNV_1_DIV_4_HZ] = 4000,
> > + [MAX31827_CNV_1_HZ] = 1000,
> > + [MAX31827_CNV_4_HZ] = 250,
> > + [MAX31827_CNV_8_HZ] = 125,
> > +};
> > +
> > struct max31827_state {
> > /*
> > * Prevent simultaneous access to the i2c client.
> > @@ -54,15 +66,13 @@ static const struct regmap_config
> max31827_regmap = {
> > .max_register = 0xA,
> > };
> >
> > -static int write_alarm_val(struct max31827_state *st, unsigned int reg,
> > - long val)
> > +static int shutdown_write(struct max31827_state *st, unsigned int reg,
> > + unsigned int val)
> > {
> > unsigned int cfg;
> > - unsigned int tmp;
> > + unsigned int cnv_rate;
> > int ret;
> >
> > - val = MAX31827_M_DGR_TO_16_BIT(val);
> > -
> > /*
> > * Before the Temperature Threshold Alarm and Alarm Hysteresis
> Threshold
> > * register values are changed over I2C, the part must be in
> > shutdown @@ -82,9 +92,10 @@ static int write_alarm_val(struct
> max31827_state *st, unsigned int reg,
> > if (ret)
> > goto unlock;
> >
> > - tmp = cfg & ~(MAX31827_CONFIGURATION_1SHOT_MASK |
> > + cnv_rate = MAX31827_CONFIGURATION_CNV_RATE_MASK & cfg;
> > + cfg = cfg & ~(MAX31827_CONFIGURATION_1SHOT_MASK |
> > MAX31827_CONFIGURATION_CNV_RATE_MASK);
> > - ret = regmap_write(st->regmap,
> MAX31827_CONFIGURATION_REG, tmp);
> > + ret = regmap_write(st->regmap,
> MAX31827_CONFIGURATION_REG, cfg);
> > if (ret)
> > goto unlock;
> >
> > @@ -92,13 +103,23 @@ static int write_alarm_val(struct
> max31827_state *st, unsigned int reg,
> > if (ret)
> > goto unlock;
> >
> > - ret = regmap_write(st->regmap,
> MAX31827_CONFIGURATION_REG, cfg);
> > + ret = regmap_update_bits(st->regmap,
> MAX31827_CONFIGURATION_REG,
> > +
> MAX31827_CONFIGURATION_CNV_RATE_MASK,
> > + cnv_rate);
> >
> > unlock:
> > mutex_unlock(&st->lock);
> > return ret;
> > }
> >
> > +static int write_alarm_val(struct max31827_state *st, unsigned int reg,
> > + long val)
> > +{
> > + val = MAX31827_M_DGR_TO_16_BIT(val);
> > +
> > + return shutdown_write(st, reg, val); }
> > +
> > static umode_t max31827_is_visible(const void *state,
> > enum hwmon_sensor_types type, u32
> attr,
> > int channel)
> > @@ -243,32 +264,7 @@ static int max31827_read(struct device *dev,
> enum
> > hwmon_sensor_types type,
> >
> > uval =
> FIELD_GET(MAX31827_CONFIGURATION_CNV_RATE_MASK,
> > uval);
> > - switch (uval) {
> > - case MAX31827_CNV_1_DIV_64_HZ:
> > - *val = 64000;
> > - break;
> > - case MAX31827_CNV_1_DIV_32_HZ:
> > - *val = 32000;
> > - break;
> > - case MAX31827_CNV_1_DIV_16_HZ:
> > - *val = 16000;
> > - break;
> > - case MAX31827_CNV_1_DIV_4_HZ:
> > - *val = 4000;
> > - break;
> > - case MAX31827_CNV_1_HZ:
> > - *val = 1000;
> > - break;
> > - case MAX31827_CNV_4_HZ:
> > - *val = 250;
> > - break;
> > - case MAX31827_CNV_8_HZ:
> > - *val = 125;
> > - break;
> > - default:
> > - *val = 0;
> > - break;
> > - }
> > + *val = max31827_conversions[uval];
> > }
> > break;
> >
> > @@ -284,6 +280,7 @@ static int max31827_write(struct device *dev,
> enum hwmon_sensor_types type,
> > u32 attr, int channel, long val)
> > {
> > struct max31827_state *st = dev_get_drvdata(dev);
> > + int res = 1;
> > int ret;
> >
> > switch (type) {
> > @@ -333,39 +330,27 @@ static int max31827_write(struct device *dev,
> enum hwmon_sensor_types type,
> > if (!st->enable)
> > return -EINVAL;
> >
> > - switch (val) {
> > - case 125:
> > - val = MAX31827_CNV_8_HZ;
> > - break;
> > - case 250:
> > - val = MAX31827_CNV_4_HZ;
> > - break;
> > - case 1000:
> > - val = MAX31827_CNV_1_HZ;
> > - break;
> > - case 4000:
> > - val = MAX31827_CNV_1_DIV_4_HZ;
> > - break;
> > - case 16000:
> > - val = MAX31827_CNV_1_DIV_16_HZ;
> > - break;
> > - case 32000:
> > - val = MAX31827_CNV_1_DIV_32_HZ;
> > - break;
> > - case 64000:
> > - val = MAX31827_CNV_1_DIV_64_HZ;
> > - break;
> > - default:
> > - return -EINVAL;
> > - }
> > + /*
> > + * Convert the desired conversion rate into
> register
> > + * bits. res is already initialized with 1.
> > + *
> > + * This was inspired by lm73 driver.
> > + */
> > + while (res < ARRAY_SIZE(max31827_conversions)
> &&
> > + val < max31827_conversions[res])
> > + res++;
> > +
> > + if (res == ARRAY_SIZE(max31827_conversions) ||
> > + val != max31827_conversions[res])
> > + return -EOPNOTSUPP;
> >
> > - val =
> FIELD_PREP(MAX31827_CONFIGURATION_CNV_RATE_MASK,
> > - val);
> > + res =
> FIELD_PREP(MAX31827_CONFIGURATION_CNV_RATE_MASK,
> > + res);
> >
> > return regmap_update_bits(st->regmap,
> >
> MAX31827_CONFIGURATION_REG,
> >
> MAX31827_CONFIGURATION_CNV_RATE_MASK,
> > - val);
> > + res);
> > }
> > break;
> >
