Please review: Draft of modular machine system

[HorstBaerbel]

Don't merge, don't use! It will not compile and would probably fry your machine if run!

This is a non-working draft of the modular machine system I was talking about. I'd like to get your feedback and have some questions before I can continue. Some explanations:

I've tried to break up _asusfan.c into manageable chunks. There's lots of stuff you'd never touch. I moved some stuff to: driver.h ->platform / driver structs msg.h -> kernel message macros machine.h / .c -> Machine interface. Probes machines. Add new machines in .c file. _machbase.h / .c -> Machine base file. Structs for defining machines. _machdefault.c ->Default machine. Old code from asus_fan.c _machux410uak.c ->UX410UAK machine. Mostly copy-pasted from _machdefault.c

There's now two central structs (in _machbase.h): _asus_fandata -> samesamebutdifferent. Stores current machine / fan states and capabilities. _asus_machinecontrol -> contains _asus_fandata, but also provides a machine abstraction by providing function pointers to methods to identify a machine, initialize it and get/set all values. Currently there are two asus_machine_control structs, in _machdefault.c and _machux410uak.c which provide machine specific data + implementations.

When the module is loaded it will call _findmachine() in machine.h, which iterates through the list of machines available and calls their _ismachine() method to find a machine match. If a machine is found, initialize() is called. This sets up the machine, finds fans and temperature sensors and enables fan auto mode. Then the regular driver code is run (I mostly copy-pasted that, refactoring a bit, sprinkled with some checks here and there).

Please give me some feedback if the structure makes sense / what is wrong / how to improve.

Some questions:

• Which of the functions _fan_get_cur_state / fan_set_cur_state / fan_get_cur_control_state / fan_set_cur_control_state / fan_get_mode / fan_setmode are still / should be used? I'm not really sure.
• I don't get how hwmon is set up? How are the functions (and which) in _asusfan.c connected to the hwmon attributes? Why are some defined obsolete?

[HorstBaerbel]

Would be nice to get your feedback

[daringer]

@HorstBaerbel, currently struggling on time to take a closer look. But will invest time today evening, highly appreciate your effort especially on generalization/re-structuring of asus-fan... Sorry for the lengthy reaction time. By beginning of October major changes will allow me, amongst other things, to invest more into asus-fan ...

[daringer]

OK let's start with the simple things:

• driver.h & msg.h are obvious to be done like this while separating, so nothing to add here

• mach_base.h is more or less deriving into _default.c or any specific _ux410uak

  • generally I am a big fan of this approach
  • machines.h provides the "needed indirection" in order to use the right file for the right laptop
  • the _<laptoptype>.c files lead to a "machine_control" struct containing anything non-default (i.e., overwritable) to realize other ACPI-calls, operations for a specific <laptoptype>.... neat

Overall this looks veeery charming, means this direction has my full support. For me one critical issue to be made sure is that there is no (or nearly no) redundant code, if one just has another ACPI call, this should not lead to the full mach_default.c being copied. Don't know if you've planned a solution for this, but I would maybe suggest to use some "mach_XXXX.c" machine_control struct as a base by copying it for the model XXXX and then just overwrite the necessary attributes ...

finally to answer you questions:

• your choice on the asus_machine_control struct members (the functions) is a good first shot, I would not care too much and go for it, will need adaptations later anyways, while more models get added
• hwmon is working pretty easy, starting from here, all DEVICE_ATTR macro calls can be safely removed, but they show you how hwmon works. In a nutshell: each hwmon file (within /sys/class/hwmon/hwmonZ/) is a "device" being created using DEVICE_ATTR() (docs)

• DEVICE_ATTR() might get up to 4 arguments (attention: just what I remember, might not be fully true):

  • name
  • (file) permissions
  • get_function_ptr (called if file is opened for reading, returns the passed *buf to userland)
  • set_function_ptr (called if someone is writing into the file with *buf being the written data)

That's actually it, not much magic behind hwmon, at least not in this driver/complexity of hw monitoring device. Being more precise: get_function_ptr above is a function-pointer to e.g., fan_get_cur_state(), means DEVICE_ATTR() glues them to a virtual hwmon device represented as file within /sys/class/hwmon/hwmonXXX.

The commented out DEVICE_ATTR() lines are there since my last cleanup/refactoring to get rid of the monolitic structure of the driver by using hwmon_attrs to represent a call to DEVICE_ATTR() if the right indices are != NULL ...

Hope could help and clarify some open topics, keep asking and nagging please ;) Hope Ryzen will finally get built into zenbooks the next months, then I'll be there again as "real" help ...

[HorstBaerbel]

Thanks for your input daringer! I will try to work on the code to iron out the kinks when I find time, probably next week. Some open topics:

• _For me one critical issue to be made sure is that there is no (or nearly no) redundant code, if one just has another ACPI call, this should not lead to the full machdefault.c being copied. I though about that too, but then got lazy. I will try to adress that.

• _I would maybe suggest to use some "mach_XXXX.c" machinecontrol struct as a base by copying it for the model XXXX and then just overwrite the necessary attributes ... Good idea. Not sure is that will work 100%, but will try something similar.

• _Which of the functions fan_get_cur_state / fan_set_cur_state / fan_get_cur_control_state / fan_set_cur_control_state / fan_get_mode / fan_setmode are still / should be used? I'm not really sure. Please be a bit more specific here. I'm not sure what functions were supposed to be used.

[daringer]

• Which of the functions fan_get_cur_state / fan_set_cur_state / fan_get_cur_control_state / fan_set_cur_control_state / fan_get_mode / fan_set_mode are still / should be used? I'm not really sure.

maybe I just misunderstood your question here, overall as far as I can currently oversee all of the fan_*_{set,get}_ functions are used, depends obviously on the laptop model's capabilities. Nevertheless I would not care too much about them leave them all inside the mach_default and just make sure that the ones needing special care are overwritten or wrapped into functions within the specific mach_XXXX. Here I would clearly suggest to keep it as simple as possible for now, refactoring/wrapping/rewriting/abstracting them can easily be done later.

I am a big fan of your work here! My old Zenbook has been passed on to a friend, I have arranged that once your work would be in a working (beta?) stage I would get it for some days to be your test-slave ;)

[HorstBaerbel]

Just a heads-up: I sold my UX3410UA, thus I can't continue working on that pull request. Sorry :/ and thanks for your work and effort!

[daringer]

Damn it! days? weeks? (hopefully not longer) before I get my new one. Hope your new one has a working fan-ctrl under Linux :D Your code-overhaul will live on, inspired me towards the right direction. Best and thanks for your contribution(s)

[HorstBaerbel]

I went fanless + Desktop ;), but thanks and good to hear this at least inspired you. Best Regards and good luck with your new device.