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