Improved IT87-support to support newer asustor devices, fix blinking status LED

[DanielGibson]

This is based on your it87-branch, and on https://github.com/phjz/AS6704T, and on the kernel source asustor released (https://sourceforge.net/projects/asgpl/files/ADM4.1.0/4.1.0.RLQ1_and_above/).

It definitely works with my AS5402T (Nimbustor 2 Gen2), and most probably also on other Jasper-Lake based NASs, like AS670xT (Lockerstor Gen2) or AS5404T. It should also work on AS660xT (Lockerstor 2/4 based on Gemini Lake); I hope @DOcGui from #3 can confirm that.

I hope that I (or the original it87 branch) didn't break anything for other devices that were previously supported by this repo; I can't test that myself.

Because mainline's gpio-it87 doesn't support IT8625E, I added a patched version (asustor-gpio-it87).

Yes, I was able to find a way to control that annoyingly blinking green status LED, see README.md.

[mafredri]

This is some really awesome work @DanielGibson 🎉, thanks for submitting the PR! I’ve been a bit busy and haven’t gotten around to reviewing/testing this, but I’ll try to have a look this week(end).

[DanielGibson]

Great, thanks! One small problem I just noticed: the usb copy key doesn't work, using the one from 6100 gives an error in dmesg: gpio-keys-polled gpio-keys-polled: unable to claim gpio 181, err=-517 so I disabled the key support for now.

If you can give me a hint how to figure out the gpio of the key, I can try to fix this properly (assuming the problem is that it's the wrong one and not that it's already in use or whatever)

Another open problem is the LAN LED, no idea which gpio it uses either, if you have an idea how to test different gpios without rebuilding the kernel module each time I could do that.

[mafredri]

@DanielGibson I'm seeing the same error on AS61, so there's a good chance that it's still the right pin/GPIO, just the wrong hard-coded base address.

❯ for c in /sys/class/gpio/gpiochip*; do echo $(<$c/label): $(<$c/base); done
INT0002 Virtual GPIO: 673
gpio_it87: 676
INT33FF:03: 740
INT33FF:02: 826
INT33FF:01: 853
INT33FF:00: 926

As you can see, the base for gpio_it87 has changed from 161 to 676 on my system. After updating the base and recompiling:

[  892.508351] input: asustor-keys as /devices/platform/gpio-keys-polled/input/input46

I never did try to figure out an alternate approach to this: https://github.com/mafredri/asustor-platform-driver/blob/e8b2a5064b99baabf0331d7f1bb8cf3f4cfd617b/asustor.c#L235-L237

It would be nice to eliminate the hard-coding since that's not portable at all.

As for the lan led, on AS61 it's line 52/GP74/physical pin 113. So you could try the matching the physical pin on the IT8625E. According to some random picture I found online, that would be GP77. You can use gpioinfo to figure out the line number and then play around with it using the export interface.

it87_chip=gpiochipXXX
echo $(($(</sys/class/gpio/$it87_chip/base) + [line_number])) > /sys/class/gpio/export
echo out > /sys/class/gpio/it87_gp77/direction
echo 1 > /sys/class/gpio/it87_gp77/value
echo 0 > /sys/class/gpio/it87_gp77/value

If that's not a match, you could try some of the surrounding pins too.

[mafredri]

Hmm, it seems that hack is no longer necessary, the gpio lookup for gpio-keys-polled seems to be working fine without it. 🤔

[DanielGibson]

Thanks for the info - I'll try that out later, just wanted to comment on one thing right now:

just the wrong hard-coded base address

I doubt that the base address is wrong, because most LEDs do work, and several still have the same offset(?) as on the AS6100, e.g. 56 for blue power and 8 for red power Update: Ok, this was nonsense, with #define AS6100_GPIO_IT87_BASE 620 the button indeed works (verified with evtest)

[DanielGibson]

OTOH:

$ for c in /sys/class/gpio/gpiochip*; do echo $(<$c/label): $(<$c/base); done
asustor_gpio_it87: 620
INT34C8:00: 684

is the base address only used for the keys, not for the LEDs?

Hmm, it seems that hack is no longer necessary, the gpio lookup for gpio-keys-polled seems to be working fine without it.

what do you mean? how should the code be changed to not use the hack?

[DanielGibson]

Updated AS6100_GPIO_IT87_BASE, but if we could get rid of it that would be even better, of course

[mafredri]

is the base address only used for the keys, not for the LEDs?

Yep, for LEDs the lookup has always been dynamic. When I last looked at this I couldn’t get the dynamic lookup for gpio keys working though, although the code for it is in place. So I added the for loop to fix it based on hard-coded base.

what do you mean? how should the code be changed to not use the hack?

Remove the for-loop below that comment I referenced earlier. If you recompile and it still works, the dynamic lookup is working as intended. Then we can remove gpio_base from asustor_driver_data and all the related defines.

[DanielGibson]

Removing that for-loop doesn't work for me, evtest doesn't detect pressing the copy button then (kernel 6.1.0-13 from debian).

[DanielGibson]

While removing the for-loop didn't work, I modified it to dynamically get the gpio_base for the chip, identified through the chip name/label/whatever from the lookup table (e.g. asustor_600_gpio_keys_lookup), like GPIO_IT87.

[DanielGibson]

The LAN LED indeed is on GP77 (assuming there is only one, and not multiple in different colors), added support for it.

I think that's all? :)

[DanielGibson]

Ok, I think now we're ready (assuming all these changes in this branch didn't break anything on older devices).

[przemekbialek]

Hi @DanielGibson son, I'm new here, but I have Asustor AS6702T, and tested Your changes. They mostly work, but I find a problem with GPIO_ACTIVE_HIGH/GPIO_ACTIVE_LOW states of leds. Green status and sata leds should be set to GPIO_ACTIVE_HIGH, and red status led should be GPIO_ACTIVE_LOW. For the sata leds it's a matter of taste, because with Your setup leds start in off state, but after disc activity they stays in on state, and with setting GPIO_ACTIVE_LOW, they start on, and after disc activity they stay in off state. I tested this also with commenting '.default_trigger = "disk-activity",' line, and with GPIO_ACTIVE_LOW and LEDS_GPIO_DEFSTATE_ON, leds are off, and setting LEDS_GPIO_DEFSTATE_OFF they light. After changing to GPIO_ACTIVE_HIGH they behave properly. I have only two sata discs so I'm not sure that the changes are corect for sata3/4 green leds. I think that the same problem have phjz, because in his repo (https://github.com/phjz/AS6704T) he set red status led to on and green to off as a default, and he has AS6704T. Thank You for Your hard work.

diff --git a/asustor.c b/asustor.c
index 14530b1..59cc62b 100644
--- a/asustor.c
+++ b/asustor.c
@@ -117,18 +117,18 @@ static struct gpiod_lookup_table asustor_6700_gpio_leds_lookup = {
        .table = {
                GPIO_LOOKUP_IDX(GPIO_IT87, 56, NULL, 0, GPIO_ACTIVE_LOW),       //blue power led
                GPIO_LOOKUP_IDX(GPIO_IT87,  8, NULL, 1, GPIO_ACTIVE_LOW),       //red power led
-               GPIO_LOOKUP_IDX(GPIO_IT87, 31, NULL, 2, GPIO_ACTIVE_LOW),       //green status led
-               GPIO_LOOKUP_IDX(GPIO_IT87, 49, NULL, 3, GPIO_ACTIVE_HIGH),      //red status led
+               GPIO_LOOKUP_IDX(GPIO_IT87, 31, NULL, 2, GPIO_ACTIVE_HIGH),      //green status led
+               GPIO_LOOKUP_IDX(GPIO_IT87, 49, NULL, 3, GPIO_ACTIVE_LOW),       //red status led
                // TODO: is there a blue USB LED in these devices?
                GPIO_LOOKUP_IDX(GPIO_IT87, 21, NULL, 5, GPIO_ACTIVE_LOW),       //green usb led
                GPIO_LOOKUP_IDX(GPIO_IT87, 55, NULL, 6, GPIO_ACTIVE_HIGH),      //blue LAN
-               GPIO_LOOKUP_IDX(GPIO_IT87, 12, NULL, 7, GPIO_ACTIVE_LOW),       //sata1 green led
+               GPIO_LOOKUP_IDX(GPIO_IT87, 12, NULL, 7, GPIO_ACTIVE_HIGH),      //sata1 green led
                GPIO_LOOKUP_IDX(GPIO_IT87, 13, NULL, 8, GPIO_ACTIVE_LOW),       //sata1 red led
-               GPIO_LOOKUP_IDX(GPIO_IT87, 46, NULL, 9, GPIO_ACTIVE_LOW),       //sata2 green led
+               GPIO_LOOKUP_IDX(GPIO_IT87, 46, NULL, 9, GPIO_ACTIVE_HIGH),      //sata2 green led
                GPIO_LOOKUP_IDX(GPIO_IT87, 47, NULL, 10, GPIO_ACTIVE_LOW),      //sata2 red led
-               GPIO_LOOKUP_IDX(GPIO_IT87, 51, NULL, 11, GPIO_ACTIVE_LOW),      //sata3 green led
+               GPIO_LOOKUP_IDX(GPIO_IT87, 51, NULL, 11, GPIO_ACTIVE_HIGH),     //sata3 green led
                GPIO_LOOKUP_IDX(GPIO_IT87, 52, NULL, 12, GPIO_ACTIVE_LOW),      //sata3 red led
-               GPIO_LOOKUP_IDX(GPIO_IT87, 63, NULL, 13, GPIO_ACTIVE_LOW),      //sata4 green led
+               GPIO_LOOKUP_IDX(GPIO_IT87, 63, NULL, 13, GPIO_ACTIVE_HIGH),     //sata4 green led
                GPIO_LOOKUP_IDX(GPIO_IT87, 48, NULL, 14, GPIO_ACTIVE_LOW),      //sata4 red led
                GPIO_LOOKUP_IDX(GPIO_IT87, 59, NULL, 15, GPIO_ACTIVE_HIGH),     //LCD power
                // sata5 green: 61, sata5 red: 62 (probably)

[DanielGibson]

Thanks for the feedback, I changed it accordingly

[DanielGibson]

Thanks for the review! I've commented on everything that needed commenting and already integrated your suggestions for the README. I'll do code changes later today (tonight).

I think the only one where I'm not certain what to do is controlling the LCD display as LED - it seems like a good idea to me (as I've written in my reply to your comment), but if you want, I'll remove that.

[DanielGibson]

Ok, I think I'm done, at least for the things that I agreed should be changed ;)

[mafredri]

Looks great, thanks for making all the changes! I'll try to get this merged today or tomorrow.

[mafredri]

Finally got around to test this on my AS-604T as well, works beautifully. Thanks for submitting the PR and all the effort you put in! ❤️ 🎉

(FYI, pushed a few small additional tweaks to main.)