From: Luke Jones <luke@ljones.dev>
To: Guenter Roeck <linux@roeck-us.net>
Cc: Hans de Goede <hdegoede@redhat.com>,
linux-kernel@vger.kernel.org, pobrn@protonmail.com,
platform-driver-x86@vger.kernel.org
Subject: Re: [PATCH v6 0/1] asus-wmi: Add support for custom fan curves
Date: Mon, 30 Aug 2021 08:55:17 +1200 [thread overview]
Message-ID: <5SCMYQ.7F7995ZKI2HT3@ljones.dev> (raw)
In-Reply-To: <2af6628e-118f-6a75-8074-2f4144c7f8e7@roeck-us.net>
On Sun, Aug 29 2021 at 08:18:01 -0700, Guenter Roeck
<linux@roeck-us.net> wrote:
> On 8/29/21 3:03 AM, Luke Jones wrote:
>>
>>
>> On Sun, Aug 29 2021 at 11:57:55 +0200, Hans de Goede
>> <hdegoede@redhat.com> wrote:
>>> Hi Luke,
>>>
>>> On 8/29/21 9:14 AM, Luke D. Jones wrote:
>>>> Add support for custom fan curves found on some ASUS ROG laptops.
>>>>
>>>> - V1
>>>> + Initial patch work
>>>> - V2
>>>> + Don't fail and remove wmi driver if error from
>>>> asus_wmi_evaluate_method_buf() if error is -ENODEV
>>>> - V3
>>>> + Store the "default" fan curves
>>>> + Call throttle_thermal_policy_write() if a curve is erased to
>>>> ensure
>>>> that the factory default for a profile is applied again
>>>> - V4
>>>> + Do not apply default curves by default. Testers have found
>>>> that the
>>>> default curves don't quite match actual no-curve behaviours
>>>> + Add method to enable/disable curves for each profile
>>>> - V5
>>>> + Remove an unrequired function left over from previous
>>>> iterations
>>>> + Ensure default curves are applied if user writes " " to a
>>>> curve path
>>>> + Rename "active_fan_curve_profiles" to
>>>> "enabled_fan_curve_profiles" to
>>>> better reflect the behavious of this setting
>>>> + Move throttle_thermal_policy_write_*pu_curves() and rename to
>>>> fan_curve_*pu_write()
>>>> + Merge fan_curve_check_valid() and fan_curve_write()
>>>> + Remove some leftover debug statements
>>>> - V6
>>>> + Refactor data structs to store array or u8 instead of
>>>> strings.
>>>> This affects the entire patch except the enabled_fan_curves
>>>> block
>>>> + Use sysfs_match_string in enabled_fan_curve block
>>>> + Add some extra comments to describe things
>>>> + Allow some variation in how fan curve input can be formatted
>>>> + Use SENSOR_DEVICE_ATTR_2_RW() to reduce the amount of lines
>>>> per
>>>> fan+profile combo drastically
>>>
>>> Thank you for your continued work on this. I read in the discussin
>>> of v5
>>> that you discussed using the standard auto_point#_pwm,
>>> auto_point#_temp
>>> pairs. I see here that you have decided to not go that route, may I
>>> ask
>>> why ?
>>
>> Sure, primary reason is because the RPM for the fans is in
>> percentage so it didn't really make sense to me to use that format.
>>
>> Also if the max for that is 255 then I'd need to introduce scaling
>> to make match what the ACPI method expects (max 100). But yeah,
>> auto_point#_pwm changes the meaning.
>>
>
> Bad argument. That is true for other controllers as well. You could
> just scale it up and down as needed.
>
> The whole point of an ABI is that it is standardized.
> If others would [be permitted to] follow your line of argument,
> we would not have a useful ABI because "my chip provides/needs
> data in some other format".
>
> Guenter
Understood. But lets see if I understand fully:
The key part is "pwmX_auto_pointN_temp and pwmX_auto_pointN_pwm", with
X being a profile, and N the point in the curve graph. If I use this
format I have:
- 3 profiles
- each profile has two fans
- each fan has 8 points on it
- each point has 2 integers
so that makes for a total of 96 individual sysfs paths. Is that really
okay? And where would the new paths god?
- Under /sys/devices/platform/asus-nb-wmi/ still, or
- /sys/devices/platform/asus-nb-wmi/hwmon/ ?
I'm currently using SENSOR_DEVICE_ATTR_2_RW with index = profile, nr =
fan. If there weren't profiles involved then I could see it being
easily achieved with that.. Maybe I could use the index(profile) with a
mask to get the fan number.
I've done all the groundwork for it at least, so it can certainly be
done. My only worry is that because of the sheer number of sysfs paths
being added (96) it could become unwieldy to use.
Could I use the existing method + the above?
Many thanks,
Luke.
next prev parent reply other threads:[~2021-08-29 20:55 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-29 7:14 [PATCH v6 0/1] asus-wmi: Add support for custom fan curves Luke D. Jones
2021-08-29 7:14 ` [PATCH v6] " Luke D. Jones
2021-08-29 9:57 ` [PATCH v6 0/1] " Hans de Goede
2021-08-29 10:03 ` Luke Jones
2021-08-29 15:18 ` Guenter Roeck
2021-08-29 20:55 ` Luke Jones [this message]
2021-08-29 22:20 ` Luke Jones
2021-08-30 9:08 ` Luke Jones
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=5SCMYQ.7F7995ZKI2HT3@ljones.dev \
--to=luke@ljones.dev \
--cc=hdegoede@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@roeck-us.net \
--cc=platform-driver-x86@vger.kernel.org \
--cc=pobrn@protonmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).