Hi Hui,
Thanks for the patch. I have few comments in the code below.
On 10/21/2016 08:33 AM, Hui Chun Ong wrote:
Add the driver to support User LEDs on PXI Embedded Controller.
Signed-off-by: Hui Chun Ong <hui.chun.ong@xxxxxx>
Signed-off-by: Brad Mouring <brad.mouring@xxxxxx>
---
drivers/leds/Kconfig | 11 +++
drivers/leds/Makefile | 1 +
drivers/leds/leds-ni78bx.c | 210 +++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 222 insertions(+)
create mode 100644 drivers/leds/leds-ni78bx.c
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 7a628c6..5540795 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -659,6 +659,17 @@ config LEDS_MLXCPLD
This option enabled support for the LEDs on the Mellanox
boards. Say Y to enabled these.
+config LEDS_NI78BX
+ tristate "LED support for NI PXI NIC78bx devices"
+ depends on LEDS_CLASS
+ depends on X86 && ACPI
+ help
+ This option enables support for the User1 and User2 LEDs on NI
+ PXI NIC78bx devices.
+
+ To compile this driver as a module, choose M here: the module
+ will be called leds-ni78bx.
+
comment "LED Triggers"
source "drivers/leds/trigger/Kconfig"
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 3965070..9758d1e 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -71,6 +71,7 @@ obj-$(CONFIG_LEDS_IS31FL319X) += leds-is31fl319x.o
obj-$(CONFIG_LEDS_IS31FL32XX) += leds-is31fl32xx.o
obj-$(CONFIG_LEDS_PM8058) += leds-pm8058.o
obj-$(CONFIG_LEDS_MLXCPLD) += leds-mlxcpld.o
+obj-$(CONFIG_LEDS_NI78BX) += leds-ni78bx.o
# LED SPI Drivers
obj-$(CONFIG_LEDS_DAC124S085) += leds-dac124s085.o
diff --git a/drivers/leds/leds-ni78bx.c b/drivers/leds/leds-ni78bx.c
new file mode 100644
index 0000000..3da4675
--- /dev/null
+++ b/drivers/leds/leds-ni78bx.c
@@ -0,0 +1,210 @@
+/*
+ * Copyright (C) 2016 National Instruments Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/acpi.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+
+#define USER1_LED_MASK 0x3
+#define USER1_GREEN_LED BIT(0)
+#define USER1_YELLOW_LED BIT(1)
+
+#define USER2_LED_MASK 0xC
+#define USER2_GREEN_LED BIT(2)
+#define USER2_YELLOW_LED BIT(3)
+
+#define LOCK_REG_OFFSET 1
+#define LOCK_VALUE 0xA5
+#define UNLOCK_VALUE 0x5A
Please add NIC78BX namespacing prefix to these macros.
+
+#define USER_LED_IO_SIZE 2
+
+static u16 io_base;
+static DEFINE_MUTEX(led_lock);
Don't define static variables but instead create a struct that
will aggregate them and allocate it dynamically.
+
+struct ni78bx_led {
+ u8 bit;
+ u8 mask;
+ struct led_classdev cdev;
+};
+
+static inline struct ni78bx_led *to_ni78bx_led(struct led_classdev *cdev)
+{
+ return container_of(cdev, struct ni78bx_led, cdev);
+}
+
+static void ni78bx_brightness_set(struct led_classdev *cdev,
+ enum led_brightness brightness)
+{
+ struct ni78bx_led *nled = to_ni78bx_led(cdev);
+ u8 value;
+
+ mutex_lock(&led_lock);
You can use spin_lock_irqsave() instead, since we are not going to sleep
while accessing memory mapped registers.
Note that in the opposite case you would have to use
brightness_set_blocking op.
+
+ value = inb(io_base);
+
+ if (brightness) {
+ value &= ~nled->mask;
+ value |= nled->bit;
+ } else {
+ value &= ~nled->bit;
+ }
+
+ outb(value, io_base);
+ mutex_unlock(&led_lock);
+}
+
+static enum led_brightness ni78bx_brightness_get(struct led_classdev *cdev)
+{
+ struct ni78bx_led *nled = to_ni78bx_led(cdev);
+ u8 value;
+
+ mutex_lock(&led_lock);
+ value = inb(io_base);
+ mutex_unlock(&led_lock);
+
+ return (value & nled->bit) ? 1 : LED_OFF;
+}
+
+static struct ni78bx_led ni78bx_leds[] = {
+ {
+ .bit = USER1_GREEN_LED,
+ .mask = USER1_LED_MASK,
+ .cdev = {
+ .name = "nilrt:green:user1",
+ .max_brightness = 1,
+ .brightness_set = ni78bx_brightness_set,
+ .brightness_get = ni78bx_brightness_get,
+ }
+ },
+ {
+ .bit = USER1_YELLOW_LED,
+ .mask = USER1_LED_MASK,
+ .cdev = {
+ .name = "nilrt:yellow:user1",
+ .max_brightness = 1,
+ .brightness_set = ni78bx_brightness_set,
+ .brightness_get = ni78bx_brightness_get,
+ }
+ },
+ {
+ .bit = USER2_GREEN_LED,
+ .mask = USER2_LED_MASK,
+ .cdev = {
+ .name = "nilrt:green:user2",
+ .max_brightness = 1,
+ .brightness_set = ni78bx_brightness_set,
+ .brightness_get = ni78bx_brightness_get,
+ }
+ },
+ {
+ .bit = USER2_YELLOW_LED,
+ .mask = USER2_LED_MASK,
+ .cdev = {
+ .name = "nilrt:yellow:user2",
+ .max_brightness = 1,
+ .brightness_set = ni78bx_brightness_set,
+ .brightness_get = ni78bx_brightness_get,
+ }
+ }
+};
+
+static acpi_status
+ni78bx_resources(struct acpi_resource *res, void *data)
+{
+ struct acpi_device *led = data;
+ u16 io_size;
+
+ switch (res->type) {
+ case ACPI_RESOURCE_TYPE_IO:
+ if (io_base != 0) {
+ dev_err(&led->dev, "too many IO resources\n");
+ return AE_ERROR;
+ }
+
+ io_base = res->data.io.minimum;
+ io_size = res->data.io.address_length;
+
+ if (io_size < USER_LED_IO_SIZE) {
+ dev_err(&led->dev, "memory region too small\n");
+ return AE_ERROR;
+ }
+
+ if (!devm_request_region(&led->dev, io_base, io_size,
+ KBUILD_MODNAME)) {
+ dev_err(&led->dev, "failed to get memory region\n");
+ return AE_ERROR;
+ }
+
+ return AE_OK;
+
+ default:
+ /* Ignore unsupported resources */
+ return AE_OK;
+ }
+}
+
+static int ni78bx_remove(struct acpi_device *pdev)
+{
+ /* Lock LED register */
+ outb(LOCK_VALUE, io_base + LOCK_REG_OFFSET);
+
+ return 0;
+}
+
+static int ni78bx_add(struct acpi_device *pdev)
+{
+ int ret, i;
+ acpi_status status;
+
+ status = acpi_walk_resources(pdev->handle, METHOD_NAME__CRS,
+ ni78bx_resources, pdev);
+
+ if (ACPI_FAILURE(status) || io_base == 0)
+ return -ENODEV;
+
+ /* Unlock LED register */
+ outb(UNLOCK_VALUE, io_base + LOCK_REG_OFFSET);
+
+ for (i = 0; i < ARRAY_SIZE(ni78bx_leds); i++) {
+ ret = devm_led_classdev_register(&pdev->dev,
+ &ni78bx_leds[i].cdev);
+ if (ret)
+ return ret;
+ }
+
+ return ret;
+}
+
+static const struct acpi_device_id led_device_ids[] = {
+ {"NIC78B3", 0},
+ {"", 0},
CC also ACPI maintainers.
+};
+MODULE_DEVICE_TABLE(acpi, led_device_ids);
+
+static struct acpi_driver led_acpi_driver = {
+ .name = KBUILD_MODNAME,
+ .ids = led_device_ids,
+ .ops = {
+ .add = ni78bx_add,
+ .remove = ni78bx_remove,
+ },
+};
+
+module_acpi_driver(led_acpi_driver);
+
+MODULE_DESCRIPTION("National Instruments PXI User LEDs driver");
+MODULE_AUTHOR("Hui Chun Ong <hui.chun.ong@xxxxxx>");
+MODULE_LICENSE("GPL v2");
You're mentioning GPL v2 or any later version in your copyright notice.
In such a case you need MODULE_LICENSE("GPL") here.
--
Best regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe linux-leds" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
