On Fri, Dec 27, 2024 at 05:57:26PM +0800, Ming Yu wrote:

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

Signed-off-by: Ming Yu <redacted>

...

...

+struct __packed nct6694_hwmon_alarm {
+	u8 smi_ctrl;
+	u8 reserved1[15];
+	struct {
+		u8 hl;
+		u8 ll;
+	} vin_limit[16];
+	struct {
+		u8 hyst;
+		s8 hl;
+	} tin_cfg[32];
+	__be16 fin_ll[10];
+	u8 reserved2[4];
+};

...

+union nct6694_hwmon_rpt {
+	u8 vin;
+	struct {
+		u8 msb;
+		u8 lsb;
+	} tin;
+	__be16 fin;
+	u8 pwm;
+	u8 status;
+};
+
+union nct6694_hwmon_msg {
+	struct nct6694_hwmon_control hwmon_ctrl;
+	struct nct6694_hwmon_alarm hwmon_alarm;
+	struct nct6694_pwm_control pwm_ctrl;
+};
+
+struct nct6694_hwmon_data {
+	struct nct6694 *nct6694;
+	struct mutex lock;
+	struct nct6694_hwmon_control hwmon_en;
+	union nct6694_hwmon_rpt *rpt;
+	union nct6694_hwmon_msg *msg;
+};

...

+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.

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

...

+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,

+
+		return nct6694_write_msg(data->nct6694, NCT6694_HWMON_MOD,
+					 NCT6694_HWMON_ALARM,
+					 sizeof(data->msg->hwmon_alarm),
+					 &data->msg->hwmon_alarm);
+	default:
+		return -EOPNOTSUPP;
+	}
+}

...