Dear Vincent and Guenter,

Thank you for reviewing,
I will change the temp_hyst to a signed variable in the next patch.


Thanks,
Ming

Guenter Roeck [off-list ref] 於 2025年1月26日 週日 下午9:18寫道：

On 1/25/25 23:42, Vincent Mailhol wrote:

quoted

On 23/01/2025 at 18:11, Ming Yu wrote:

quoted

This driver supports Hardware monitor functionality for NCT6694 MFD
device based on USB interface.

Signed-off-by: Ming Yu <redacted>
---

(...)

quoted

+static int nct6694_temp_write(struct device *dev, u32 attr, int channel,
+                          long val)
+{
+    struct nct6694_hwmon_data *data = dev_get_drvdata(dev);
+    struct nct6694_cmd_header cmd_hd;
+    unsigned char temp_hyst;
+    signed char temp_max;
+    int ret;
+
+    guard(mutex)(&data->lock);
+
+    switch (attr) {
+    case hwmon_temp_enable:
+            if (val == 0)
+                    data->hwmon_en.tin_en[channel / 8] &= ~BIT(channel % 8);
+            else if (val == 1)
+                    data->hwmon_en.tin_en[channel / 8] |= BIT(channel % 8);
+            else
+                    return -EINVAL;
+
+            cmd_hd = (struct nct6694_cmd_header) {
+                    .mod = NCT6694_HWMON_MOD,
+                    .cmd = NCT6694_HWMON_CONTROL,
+                    .sel = NCT6694_HWMON_CONTROL_SEL,
+                    .len = cpu_to_le16(sizeof(data->hwmon_en))
+            };
+
+            return nct6694_write_msg(data->nct6694, &cmd_hd,
+                                     &data->hwmon_en);
+    case hwmon_temp_max:
+            cmd_hd = (struct nct6694_cmd_header) {
+                    .mod = NCT6694_HWMON_MOD,
+                    .cmd = NCT6694_HWMON_ALARM,
+                    .sel = NCT6694_HWMON_ALARM_SEL,
+                    .len = cpu_to_le16(sizeof(data->msg->hwmon_alarm))
+            };
+            ret = nct6694_read_msg(data->nct6694, &cmd_hd,
+                                   &data->msg->hwmon_alarm);
+            if (ret)
+                    return ret;
+
+            val = clamp_val(val, -127000, 127000);
+            data->msg->hwmon_alarm.tin_cfg[channel].hl = temp_to_reg(val);
+
+            return nct6694_write_msg(data->nct6694, &cmd_hd,
+                                     &data->msg->hwmon_alarm);
+    case hwmon_temp_max_hyst:
+            cmd_hd = (struct nct6694_cmd_header) {
+                    .mod = NCT6694_HWMON_MOD,
+                    .cmd = NCT6694_HWMON_ALARM,
+                    .sel = NCT6694_HWMON_ALARM_SEL,
+                    .len = cpu_to_le16(sizeof(data->msg->hwmon_alarm))
+            };
+            ret = nct6694_read_msg(data->nct6694, &cmd_hd,
+                                   &data->msg->hwmon_alarm);
+
+            val = clamp_val(val, -127000, 127000);
+            temp_max = data->msg->hwmon_alarm.tin_cfg[channel].hl;
+            temp_hyst = temp_max - temp_to_reg(val);
+            temp_hyst = clamp_val(temp_hyst, 0, 7);

temp_hyst is unsigned. It can not be smaller than zero. No need for
clamp(), using min here is sufficient.

Wrong conclusion. It needs to be declared as signed variable because
"temp_max - temp_to_reg(val)" could be negative.

Guenter