ipilcher / n5550

Thecus N5550 hardware support
GNU General Public License v2.0
8 stars 8 forks source link

A better way to automatically load n5550 leds modules #10

Open manover opened 7 years ago

manover commented 7 years ago

A simpler way to load the modules automatically. Has been tested on my system for a while. Actually, i prefer to build my own kernels, so I build the n5550 modules in-tree and also patch ahci.c and libahci.c, so there is no need for a modprobe configuration. But I realize, it's not gonna work for somebody who builds these modules out of tree. About the change in GPIO base - it switched to 450 around 4.3.y kernels and probably should be left as is if you run something older than this. Or maybe there should be an #ifdef or even if condition - not sure.

ipilcher commented 7 years ago

Interesting.

I want to make sure that I understand what this does. Am I correct that the MODULE_ALIAS("dmi:...") causes the module to get loaded?

Re the GPIO base, unfortunately it's still 195 on EL 7.3, so I don't want to change it. What does seem doable, however, is to automatically set the correct base. Can you try this udev rule?

ACTION=="add", SUBSYSTEM=="gpio", DEVPATH=="/devices/pci0000:00/0000:00:1f.0/*", RUN+="/usr/sbin/modprobe n5550_board ich_gpio_base=$attr{base}"

(I have the sense that the module should be able to do this itself with the new descriptor GPIO descriptor interface, but I'll be darned if I can find an example of how to actually use it.)

manover commented 7 years ago

Yes, it's a glob that matches the machine dmi string. This is mine:

root@snorlax:~# cat /sys/class/dmi/id/modalias
dmi:bvnPhoenixTechnologiesLtd.:bvrCDV_T30X64:bd09/17/2012:svnIntelCorporation:pnMilsteadPlatform:pvr2.2:rvnIntelCorporation:rnGraniteWell:rvrFABA:cvnIntelCorporation:ct9:cvr1.0:

bvn stands for BIOS Vendor, bvn - BIOS Version, pn - Product Name, rn - Baseboard Name, rvr - Baseboard Version, ct - Chassis Type (9 = laptop, by the way), etc.

Yours should be either identical or very close. I used wild cards for BIOS version just to be safe and removed the fields having obviously fake values. Could you paste what you see on your machine, please? I still have doubts about the board version, maybe it should be globbed, too.

Can you try this udev rule?

Yeah, it works with a little bit modified path. With the old path the rule also triggers something residing at /sys/devices/pci0000:00/0000:00:1f.0/gpio_ich.1.auto/gpiochip0.

ACTION=="add|change", SUBSYSTEM=="gpio", DEVPATH=="/devices/pci0000:00/0000:00:1f.0/gpio_ich.1.auto/gpio/*", RUN+="/usr/sbin/modprobe n5550_board ich_gpio_base=$attr{base}"

But the idea is to make it load automatically, without udev rules, ideally without modprobe config even. In the meantime, we can add an option to modprobe. Please take a look at the updated PR.

What does seem doable, however, is to automatically set the correct base.

I glanced over the **Documentation/gpio/*** and, honestly, cannot say I found anything useful. Could you point me in the right direction? In any direction at all? :)

(I have the sense that the module should be able to do this itself with the new descriptor GPIO descriptor interface, but I'll be darned if I can find an example of how to actually use it.)

Isn't this new API supposed to be used exclusively for all new GPIO drivers/GPIO consumers?

ipilcher commented 7 years ago

Here's my DMI string:

dmi:bvnPhoenixTechnologiesLtd.:bvrCDV_T30X64:bd09/17/2012:svnIntelCorporation:pnMilsteadPlatform:pvr2.2:rvnIntelCorporation:rnGraniteWell:rvrFABA:cvnIntelCorporation:ct9:cvr1.0:

But the idea is to make it load automatically, without udev rules, ideally without modprobe config even. In the meantime, we can add an option to modprobe. Please take a look at the updated PR.

I've taken a look, but I don't see how you're working around the "dynamic base" issue. Did I just miss it?

I glanced over the Documentation/gpio/* and, honestly, cannot say I found anything useful. Could you point me in the right direction? In any direction at all? :)

I wish I could. All of the documentation seems to be oriented toward device tree-based systems. I just have this gut feeling that the "right way" to deal with the variable GPIO number is to not use GPIO numbers at all.