| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <44e84cb8-b573-0e1a-91e5-cdee0441d0f9@roeck-us.net>
Date: Fri, 15 Sep 2023 16:26:04 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Daniel Matyas <daniel.matyas@...log.com>
Cc: Jean Delvare <jdelvare@...e.com>, Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>,
Jonathan Corbet <corbet@....net>, linux-hwmon@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-doc@...r.kernel.org
Subject: Re: [PATCH v3 1/5] hwmon: max31827: Make code cleaner
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
> 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@...log.com>
> ---
>
> 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;
>
Powered by blists - more mailing lists