Hi
2021. augusztus 22., vasárnap 20:10 keltezéssel, Enver Balalic írta:
> [...]
> > > > +static int hp_wmi_hwmon_init(void)
> > > > +{
> > > > + struct device *dev = &hp_wmi_platform_dev->dev;
> > > > + struct device *hwmon;
> > > > +
> > > > + hwmon = devm_hwmon_device_register_with_groups(dev, "hp", &hp_wmi_driver,
> > > > + hwmon_attribute_groups);
> > >
> > > I think you should use
> > >
> > > [devm_]hwmon_device_register_with_info()
> > >
> > > as it creates sysfs attributes for you, etc. You wouldn't need to manually
> > > create device attributes, and you wouldn't need fan{1,2}_show() at all.
> > >
> >
> > Correct. Also, the code as written doesn't really make sense as hwmon driver
> > because all its attributes are non-standard. The "sensors" command will show
> > nothing.
> >
>
> I failed to take a look at the documentation instead i just looked at a
> different laptop's WMI driver as a reference. My bad.
>
> > If you don't want to use standard hwmon ABI attributes, please don't register
> > a hwmon driver. The non-standard attributes can reside in the platform
> > sysfs directory. There is already the non-standard "hddtemp", so that would
> > not make much of a difference anyway. On top of that, "max_fan" is added as
> > non-hwmon attribute, making the hwmon registration even more pointless.
> >
> > Guenter
>
> I would like to expose these fan speeds properly, and hwmon seemed like
> the way to do it. I'll redo this part to match how it should be done in the
> documentation.
>
> The questions I have are:
>
> The only value I have is the current fan speed in RPM, I don't have
> the rest of the values like min,max,pulses,target. Is it ok to implement this
> in hwmon if I don't have those values? Can I use default values in place of
> the ones I don't have, or should i omit those fields entirely.
> I can assume min speed to be 0 since it does turn off the fans at light load
> but in the case of the max speed property, I don't know what it is. Do I omit
> that field entirely or ?
>
I think you can simply omit what you don't have. In your case,
I imagine you'll have something like this:
static const struct hwmon_channel_info *info[] = {
HWMON_CHANNEL_INFO(fan, HWMON_F_INPUT, HWMON_F_INPUT),
HWMON_CHANNEL_INFO(pwm, HWMON_PWM_ENABLE),
NULL
};
static const struct hwmon_ops ops = {
.is_visible = ...,
.read = ...,
.write = ...,
};
static const struct hwmon_chip_info chip_info = {
.ops = &ops,
.info = info,
};
> The "max_fan" that I added is not a RPM value, it's a simple toggle in WMI
> that either sets the fans to blow at their max speed (1), or it sets them
> to auto (0), this matches what the windows omen command center does.
> I don't know of a way to expose this "properly" so i just added a simple
> attribute. Is there a proper way to expose this behaviour ?
>
I believe the proper way to expose that is via a pwmN_enable attribute on the
hwmon device, this should be created by the hwmon subsystem when it sees the
HWMON_CHANNEL_INFO(pwm, HWMON_PWM_ENABLE),
channel in the info struct.
If both fans are controlled by one switch, I think it's fine to just have a
single pwmN_enable attribute. But Guenter can probably tell you more.
For this attribute the value 2 means automatic, 1 means manual, 0 means full speed
as far as I'm aware (at least this is what thinkpad_acpi does). If manual fan
control is not available, returning -EOPNOTSUPP is probably fine.
> [...]
Best regards,
Barnabás Pőcze
