Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Slimbook Compatibilty addition #12

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

MarSlimbook
Copy link

No description provided.

@MarSlimbook MarSlimbook deleted the slimbook branch May 31, 2022 13:42
@MarSlimbook MarSlimbook restored the slimbook branch May 31, 2022 13:58
@MarSlimbook
Copy link
Author

My apologies, I did it accidentally. Maybe we can come up with something in the future.

@MarSlimbook MarSlimbook changed the title Fetch Lt-Henry Slimbook Compatibilty addition Jun 3, 2022
@MarSlimbook MarSlimbook reopened this Jun 6, 2022
@MarSlimbook
Copy link
Author

This adds compatibility for:

  • PROX-AMD
  • PROX15-AMD
  • PROX-AMD5
  • PROX15-AMD5

Also adds silent-mode feature for this models.

pdev.c Outdated
@@ -224,6 +292,8 @@ static DEVICE_ATTR_RW(fan_always_on);
static DEVICE_ATTR_RW(fan_reduced_duty_cycle);
static DEVICE_ATTR_RW(manual_control);
static DEVICE_ATTR_RW(super_key_lock);
static DEVICE_ATTR_RW(fan_boost);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure a new attribute is needed, see: https://github.com/torvalds/linux/blob/f2906aa863381afb0015a9eb7fefad885d4e5a56/Documentation/ABI/testing/sysfs-class-hwmon#L289 ? Enabling fan boost is already implemented via the pwm1_enable attribute of the hwmon device created in hwmon_pwm.c, which uses fan.c:qc71_fan_set_mode() to turn it on, although that writes the FAN_CTRL register directly. I am now wondering if using the TRIGGER_1 register might be better?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if fan_boost is the same as a 100% duty cycle pwm... then yes, exposing fan_boost is not necessary.

I am now wondering if using the TRIGGER_1 register might be better?

On our tests, switch through TRIGGER_1 triggers a WMI event. Tested with super key, fan boost and lightbar. On trigger 2 If I am not wrong, we could increase/decrease keyboard backlight. I can double check if you find interesting to have it documented.

That is the only benefit we found of trigger register. Unfortunately, TRIGGER_1_SILENT_MODE doesn't seem to switch what we call "Silent Mode" so we do it by writing FAN_CTRL instead.

Also, I've noticed fan.c functions are atomic, protected by a mutex and pdev.c aren't. As this is triggered by an human action (pressing Fn+F5) race conditions seem unlikely... however, we can move "silent mode" code to fan.c and surround it with a mutex

features.h Outdated Show resolved Hide resolved
@pobrn
Copy link
Owner

pobrn commented Jun 7, 2022

Another thing, please get rid of the merge commits and squash some commits (e.g. fe0b15e could be squashed into 661d9a1, etc.)

* Added Slimbook silent mode sysfs access
@Lt-Henry
Copy link

Lt-Henry commented Jun 8, 2022

Sorry for the mess, I haven't much experience with this on github. I expected PR to be smarter.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants