Re: [PATCH v4 6/7] hwmon: Add Nuvoton NCT6694 HWMON support

[Simon Horman]

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 <a0282524688@xxxxxxxxx>

...

> diff --git a/drivers/hwmon/nct6694-hwmon.c b/drivers/hwmon/nct6694-hwmon.c

...

> +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;
> +	}
> +}

...