Re: hwmon: (nct7802) buggy VSEN1/2/3 alarm

Guenter Roeck
2019-11-27T19:32:25Z

On 11/27/19 6:41 AM, Gilles Buloz wrote:
> According to your suggestions, I've made and tested this patch that works :
>
> --- nct7802.c.orig 2019-11-26 10:37:08.753693088 +0100
> +++ nct7802.c 2019-11-27 15:15:51.000000000 +0100
> @@ -32,8 +32,8 @@
> static const u8 REG_VOLTAGE[5] = { 0x09, 0x0a, 0x0c, 0x0d, 0x0e };
>
> static const u8 REG_VOLTAGE_LIMIT_LSB[2][5] = {
> - { 0x40, 0x00, 0x42, 0x44, 0x46 },
> - { 0x3f, 0x00, 0x41, 0x43, 0x45 },
> + { 0x46, 0x00, 0x40, 0x42, 0x44 },
> + { 0x45, 0x00, 0x3f, 0x41, 0x43 },
> };
>
> static const u8 REG_VOLTAGE_LIMIT_MSB[5] = { 0x48, 0x00, 0x47, 0x47, 0x48 };
> @@ -67,6 +67,7 @@
> struct nct7802_data {
> struct regmap *regmap;
> struct mutex access_lock; /* for multi-byte read and write operations */
> + u8 in_status_cache;
> };
>
> static ssize_t show_temp_type(struct device *dev, struct device_attribute *attr,
> @@ -377,6 +378,56 @@
> return err ? : count;
> }
>
> +static ssize_t show_in_alarm(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr);
> + struct nct7802_data *data = dev_get_drvdata(dev);
> + int volt, min, max, ret, i;
> + unsigned int val;
> +
> + /*
> + * The SMI Voltage statys register is the only register giving a status
> + * for volatges. A bit is set for each input crossing a threshold, in
> + * both direction, but the "inside" or "outside" limits info is not
> + * available. Also this register is cleared on read.

Might add a note that this is form experiment, and not explicitly spelled
out in the datasheet.

> + * To deal with this we use a status cache with one validity bit and
> + * one status bit for each input. Validity is cleared at startup and
> + * each time the register reports a change, and the status is processed
> + * by software based on current value and limits.
> + */
> + ret = regmap_read(data->regmap, 0x1E, &val); /* SMI Voltage status */

We are using lowercase for hex numbers elsewhere in this driver,
so please use lowercase here as well.

> + if (ret < 0)
> + return ret;
> +
> + /* if status of an input has changed, invalidate its cached status */
> + for (i=0; i<=3; i++)

Spaces before and after assignments, please.

> + if (val & (1 << i))
> + data->in_status_cache &= ~(0x10 << i);
> +
> + /* if cached status for requested input is invalid, update it */
> + if (!(data->in_status_cache & (0x10 << sattr->index))) {
> + volt = nct7802_read_voltage(data, sattr->nr, 0);
> + if (volt < 0)
> + return volt;
> + min = nct7802_read_voltage(data, sattr->nr, 1);
> + if (min < 0)
> + return min;

Do we need to check min ? The register description suggests "over limit".
No idea though what the min limits are for in that case. Might be worth
running some checks to understand this better (if you did not do that
already).

> + max = nct7802_read_voltage(data, sattr->nr, 2);
> + if (max < 0)
> + return max;
> +
> + if ((volt < min) || (volt > max))

Unnecessary inner ( )

Guenter