| Message ID | 20160606004406.29f016de@gmail.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Jun 6, 2016 at 2:44 AM, Vincent Pelletier <plr.vincent@gmail.com> wrote: > (please keep me CC'ed in replies, I'm not getting a majordomo > subscription confirmation mail) Please put me on To: on patches related to GPIO... > 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 ? 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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 14 Jun 2016 09:17:18 +0200, Linus Walleij <linus.walleij@linaro.org> wrote: > 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. 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,
On Wed, Jun 15, 2016 at 2:45 AM, Vincent Pelletier <plr.vincent@gmail.com> wrote: > 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 ? 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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On Fri, 17 Jun 2016 23:02:47 +0200, Linus Walleij <linus.walleij@linaro.org> wrote: > 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. 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,
On Sat, Jun 18, 2016 at 9:18 AM, Vincent Pelletier <plr.vincent@gmail.com> wrote: > 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. 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 -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig index ed2004b..4da05bc 100644 --- a/drivers/platform/x86/Kconfig +++ b/drivers/platform/x86/Kconfig @@ -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 diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile index 448443c..e03c507 100644 --- a/drivers/platform/x86/Makefile +++ b/drivers/platform/x86/Makefile @@ -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 diff --git a/drivers/platform/x86/qnap-tsx51.c b/drivers/platform/x86/qnap-tsx51.c new file mode 100644 index 0000000..c6304b9 --- /dev/null +++ b/drivers/platform/x86/qnap-tsx51.c @@ -0,0 +1,168 @@ +/* + * Support for LEDs and buttons available on the QNAP TS-x51 NAS. + * + * Copyright (C) 2015 Vincent Pelletier <plr.vincent@gmail.com> + */ + +#include <linux/leds.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/input.h> +#include <linux/gpio_keys.h> + +static void qnap_tsx51_device_pdev_release(struct device *dev); + +static struct gpio_led qnap_tsx51_led[] = { + { + .name = "qnap_tsx51:green:status", + .gpio = 62, + .active_low = 1, + .default_state = LEDS_GPIO_DEFSTATE_ON, + }, + { + .name = "qnap_tsx51:red:status", + .gpio = 63, + .active_low = 1, + }, + { + .name = "qnap_tsx51:blue:usb", + .default_trigger = "usb-host", + .gpio = 17, + .active_low = 1, + }, + { + .name = "hdd1:red:sata", + .gpio = 70, + .active_low = 1, + }, + { + .name = "hdd2:red:sata", + .gpio = 71, + .active_low = 1, + }, + { + .name = "hdd3:red:sata", + .gpio = 72, + .active_low = 1, + }, + { + .name = "hdd4:red:sata", + .gpio = 73, + .active_low = 1, + }, + { + .name = "hdd5:red:sata", + .gpio = 74, + .active_low = 1, + }, + { + .name = "hdd6:red:sata", + .gpio = 75, + .active_low = 1, + }, + { + .name = "hdd7:red:sata", + .gpio = 76, + .active_low = 1, + }, + { + .name = "hdd8:red:sata", + .gpio = 77, + .active_low = 1, + }, +}; + +static struct gpio_led_platform_data qnap_tsx51_led_data = { + .num_leds = ARRAY_SIZE(qnap_tsx51_led), + .leds = qnap_tsx51_led, +}; + +static struct platform_device qnap_tsx51_leds_dev = { + .name = "leds-gpio", + .id = -1, + .dev = { + .release = qnap_tsx51_device_pdev_release, + .platform_data = &qnap_tsx51_led_data, + }, +}; + +static struct gpio_keys_button qnap_tsx51_gpio_buttons[] = { + { + .code = KEY_COPY, + .gpio = 12, + .active_low = 1, + .desc = "Copy button", + .type = EV_KEY, + .wakeup = 0, + .debounce_interval = 100, + .can_disable = 0, + }, + { + .code = KEY_RESTART, + .gpio = 61, + .active_low = 1, + .desc = "Reset button", + .type = EV_KEY, + .wakeup = 0, + .debounce_interval = 100, + .can_disable = 0, + }, +}; + +static struct gpio_keys_platform_data qnap_tsx51_buttons_data = { + .buttons = qnap_tsx51_gpio_buttons, + .nbuttons = ARRAY_SIZE(qnap_tsx51_gpio_buttons), + .poll_interval = 20, +}; + +static struct platform_device qnap_tsx51_buttons_dev = { + .name = "gpio-keys-polled", + .id = -1, + .dev = { + .release = qnap_tsx51_device_pdev_release, + .platform_data = &qnap_tsx51_buttons_data, + }, +}; + +static struct platform_device *qnap_tsx51_devs[] = { + &qnap_tsx51_buttons_dev, + &qnap_tsx51_leds_dev, +}; + +static void qnap_tsx51_device_pdev_release(struct device *dev) +{ +/* + * Needed to silence this message: + * Device 'xxx' does not have a release() function, it is broken and must be + * fixed. + */ +} + +static int __init qnap_tsx51_init(void) +{ + int ret; + + ret = request_module("gpio_f7188x"); + if (ret) + return ret; + + return platform_add_devices( + qnap_tsx51_devs, + ARRAY_SIZE(qnap_tsx51_devs) + ); + +} + +static void __exit qnap_tsx51_exit(void) +{ + int i; + for (i = 0; i < ARRAY_SIZE(qnap_tsx51_devs); i++) + platform_device_unregister(qnap_tsx51_devs[i]); +} + +module_init(qnap_tsx51_init); +module_exit(qnap_tsx51_exit); + +MODULE_LICENSE("GPL"); +MODULE_DESCRIPTION("QNAP TS-x51 NAS"); +MODULE_AUTHOR("Vincent Pelletier <plr.vincent@gmail.com>");