Dear Simon,
Thank you for your comments,
Simon Horman <horms@xxxxxxxxxx> 於 2025年1月6日 週一 下午9:51寫道：
>
...
> > +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.
> > +
> > + 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,
>
Understood. Fix it in v5.
> ...
Best regards,
Ming
