Dear Simon,

Thank you for your comments,

Simon Horman [off-list ref] 於 2025年1月6日 週一 下午9:51寫道：

...

quoted

+static int nct6694_pwm_read(struct device *dev, u32 attr, int channel,
+                         long *val)
+{
+     struct nct6694_hwmon_data *data = dev_get_drvdata(dev);
+     unsigned char pwm_en;
+     int ret;
+
+     guard(mutex)(&data->lock);
+
+     switch (attr) {
+     case hwmon_pwm_enable:
+             pwm_en = data->hwmon_en.pwm_en[channel / 8];
+             *val = !!(pwm_en & BIT(channel % 8));
+
+             return 0;
+     case hwmon_pwm_input:
+             ret = nct6694_read_msg(data->nct6694, NCT6694_RPT_MOD,
+                                    NCT6694_PWM_IDX(channel),
+                                    sizeof(data->rpt->fin),
+                                    &data->rpt->fin);
+             if (ret)
+                     return ret;
+
+             *val = data->rpt->fin;

Hi Ming Yu,

*val is host byte order, but fin is big endian.
Elsewhere in this patch this seems to be handled using,
which looks correct to me:

                *val = be16_to_cpu(data->rpt->fin);

Flagged by Sparse.

Yes, it needs to be fixed to be16_to_cpu(). I'll make the modification
in the next patch.

quoted

+
+             return 0;
+     case hwmon_pwm_freq:
+             *val = NCT6694_FREQ_FROM_REG(data->hwmon_en.pwm_freq[channel]);
+
+             return 0;
+     default:
+             return -EOPNOTSUPP;
+     }
+}

...

quoted

+static int nct6694_fan_write(struct device *dev, u32 attr, int channel,
+                          long val)
+{
+     struct nct6694_hwmon_data *data = dev_get_drvdata(dev);
+     int ret;
+
+     guard(mutex)(&data->lock);
+
+     switch (attr) {
+     case hwmon_fan_enable:
+             if (val == 0)
+                     data->hwmon_en.fin_en[channel / 8] &= ~BIT(channel % 8);
+             else if (val == 1)
+                     data->hwmon_en.fin_en[channel / 8] |= BIT(channel % 8);
+             else
+                     return -EINVAL;
+
+             return nct6694_write_msg(data->nct6694, NCT6694_HWMON_MOD,
+                                      NCT6694_HWMON_CONTROL,
+                                      sizeof(data->msg->hwmon_ctrl),
+                                      &data->hwmon_en);
+     case hwmon_fan_min:
+             ret = nct6694_read_msg(data->nct6694, NCT6694_HWMON_MOD,
+                                    NCT6694_HWMON_ALARM,
+                                    sizeof(data->msg->hwmon_alarm),
+                                    &data->msg->hwmon_alarm);
+             if (ret)
+                     return ret;
+
+             val = clamp_val(val, 1, 65535);
+             data->msg->hwmon_alarm.fin_ll[channel] = (u16)cpu_to_be16(val);

cpu_to_be16() returns a 16bit big endian value.
And, AFAIKT, the type of data->msg->hwmon_alarm.fin_ll[channel] is __be16.

So the cast above seems both unnecessary and misleading.

Also flagged by Sparse,

Understood. Fix it in v5.

...

Best regards,
Ming