qnap-tsx51: add new driver for leds and button support on QNAP TS-x51 series

Hello,

(please keep me CC'ed in replies, I'm not getting a majordomo subscription confirmation mail)

I own a QNAP TS-651 NAS enclosure (the 6-drives variant of the TS-x51 product family). It is a regular x86-64, and it can work with vanilla kernel & generic distro as a replacement to vendor firmware. ACPI tables do not declare GPIOs used to interface with the enclosure:
- two buttons for input
- one red led per disk slot (for failure events)
- USB activity led
- two-colours status led (for off/green/orange/red result)

I wrote the following platform driver, that I would like to contribute to the kernel.

Could someone please review below patch ?
It is my first attempt at writing a module from scratch, so watch out for naive mistakes.

I see drivers using GPIOs should use descriptors instead of gpio number. I fail to see how I should convert this driver to use descriptors. Or at least, I like a lot that this module has extremely little actual code, as I would expect from something which essentially does the same work as ACPI tables. Is it also achievable with descriptors ?

About disk leds, there exist 2, 4, 6 and 8-drives versions of this enclosure. I do not know how to detect which one the module is being loaded on, so I declare all 8 leds, and userland should drive leds which match detected drives.
For completeness, I attached udev configuration file (to have device symlinks correctly mapping SCSI devices to slots - 6 drives version) and "mdadm --monitor --program"-compatible led control shell script (which depends on udev disk symlinks and led names being consistent).

Regards,
Vincent Pelletier

From 8c59f4f1c4f0c69b9168979fdf130b301b1be20b Mon Sep 17 00:00:00 2001
Message-Id: <8c59f4f1c4f0c69b9168979fdf130b301b1be20b.1465170970.git.plr.vincent@gmail.com>
From: Vincent Pelletier <[email protected]>
Date: Mon, 17 Aug 2015 18:50:13 +0200
Subject: qnap-tsx51: add new driver for leds and button support on QNAP TS-x51
series

This adds QNAP TS-x51 driver. It exposes led and buttons present on this NAS enclosure to userland for convenient access. Ideally, all this driver does should be declared by ACPI, but even SuperIO GPIO pins are not. Also, ideally this driver should check DMI strings, but OEM did not initialise any.

---
 drivers/platform/x86/Kconfig      |   9 ++
 drivers/platform/x86/Makefile     |   1 +
 drivers/platform/x86/qnap-tsx51.c | 168 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 178 insertions(+)
 create mode 100644 drivers/platform/x86/qnap-tsx51.c

index ed2004b..4da05bc 100644
@@ -1001,4 +1001,13 @@ config INTEL_TELEMETRY
 	  used to get various SoC events and parameters
 	  directly via debugfs files. Various tools may use
 	  this interface for SoC state monitoring.
+
+config QNAP_TSX51
+	tristate "QNAP TS-x51 NAS"
+	select LEDS_GPIO
+	select KEYBOARD_GPIO_POLLED
+	select GPIO_F7188X
+	---help---
+	  This driver provides support for QNAP TS-x51 NAS enclosure
+	  leds (drive error, status, usb) and buttons.
 endif # X86_PLATFORM_DEVICES

index 448443c..e03c507 100644
@@ -69,3 +69,4 @@ obj-$(CONFIG_INTEL_PUNIT_IPC)  += intel_punit_ipc.o
 obj-$(CONFIG_INTEL_TELEMETRY)	+= intel_telemetry_core.o \
 				   intel_telemetry_pltdrv.o \
 				   intel_telemetry_debugfs.o
+obj-$(CONFIG_QNAP_TSX51)	+= qnap-tsx51.o

2.8.1

Please put me on To: on patches related to GPIO...

Yes, you can have static mappings, it's a bit tricky and only done in a few places in the kernel. You have to add a static lookup table using #include <linux/gpio/machine.h>

See:
arch/arm/mach-integrator/impd1.c function impd1_probe() for an example.

git grep gpiod_add_lookup_table
for other examples.

Yours,
Linus Walleij

Thanks for the examples.

I notice that no driver using gpiod_add_lookup_table also uses gpiod_get for the gpios it just declared (output reordered & annotated):

  $ git grep -l gpiod_add_lookup_table | xargs grep -l gpiod_get
  Documentation/gpio/board.txt
  drivers/gpio/gpiolib.c
  include/linux/gpio/machine.h
    All above are on the gpio subsystem side, so it makes sense to
    reference both.
  drivers/mfd/intel_soc_pmic_core.c
    This one declares a "panel" descriptor and gets one for
    "intel_soc_pmic", so it is not using the descriptor it declares.
  $

So I wonder if it makes sense at all for a driver to both define a gpio lookup table and then look for each gpio it just defined.

Maybe I am doing it wrong and/or at the wrong level ?

I looked at the ACPI API and try to find a way to extend the incomplete tables when my platform module gets loaded. It seems possible with devicetree (drivers/of/dynamic.c) but I couldn't find a way with APCI yet. I did find that it's possible to override acpi entire tables, but that seems overkill for my needs (it's not that the existing tables are wrong, they are incomplete) and hard to maintain (depends on bios revision).

Regards,

Vincent Pelletier

Well ask yourself what you are doing I guess, GPIO is General Purpose Input Output, a trap some people fall into is confusing that with their special purpose input/outputs, and sometimes they are input-only or output-only on top of that. That is not GPIO.

So what is the use case for these electronics? If it is not supposed to be used by misc input/output, it is not GPIO, and it is better for the driver to manage it at will.

Yours,
Linus Walleij

Hello,

Thanks, I think it is the distinction I did not do.

Just to confirm my understanding:
- from the chip (and its module) point of view, these signals are GPIOs, because they cannot presume how it is wired
- but from the platform point of view (this specific NAS model), these GPIOs are wired to leds and buttons, so they are leds and buttons and not GPIOs anymore

So using GPIO numbers is acceptable in such driver.

Is this correct ?
If it is, I will submit my patch on platform-drivers ML.

Regards,

Vincent Pelletier

We usually model this as GPIO if it is GPIO on the SoC, because we want to reuse the same code on several systems using the same SoC.

But you should use GPIO descriptors, not numbers.

Yours,
Linus Walleij