* [PATCH] leds: Add user LED driver for NIC78bx device
@ 2016-10-21 6:33 ` Hui Chun Ong
2016-10-21 12:56 ` Jacek Anaszewski
0 siblings, 1 reply; 8+ messages in thread
From: Hui Chun Ong @ 2016-10-21 6:33 UTC (permalink / raw)
To: rpurdie, j.anaszewski
Cc: linux-leds, jonathan.hearn, julia.cartwright, Hui Chun Ong, Brad Mouring
Add the driver to support User LEDs on PXI Embedded Controller.
Signed-off-by: Hui Chun Ong <hui.chun.ong@ni.com>
Signed-off-by: Brad Mouring <brad.mouring@ni.com>
---
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
+
+#define USER_LED_IO_SIZE 2
+
+static u16 io_base;
+static DEFINE_MUTEX(led_lock);
+
+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);
+
+ 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},
+};
+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@ni.com>");
+MODULE_LICENSE("GPL v2");
--
2.7.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] leds: Add user LED driver for NIC78bx device
2016-10-21 6:33 ` [PATCH] leds: Add user LED driver for NIC78bx device Hui Chun Ong
@ 2016-10-21 12:56 ` Jacek Anaszewski
2016-10-21 13:06 ` Mika Westerberg
2016-10-21 21:33 ` Julia Cartwright
0 siblings, 2 replies; 8+ messages in thread
From: Jacek Anaszewski @ 2016-10-21 12:56 UTC (permalink / raw)
To: Hui Chun Ong, rpurdie
Cc: linux-leds, jonathan.hearn, julia.cartwright, Brad Mouring,
Mika Westerberg, Rafael J. Wysocki
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@ni.com>
> Signed-off-by: Brad Mouring <brad.mouring@ni.com>
> ---
> 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@ni.com>");
> +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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] leds: Add user LED driver for NIC78bx device
2016-10-21 12:56 ` Jacek Anaszewski
@ 2016-10-21 13:06 ` Mika Westerberg
2016-10-21 21:11 ` Rafael J. Wysocki
2016-10-21 21:33 ` Julia Cartwright
1 sibling, 1 reply; 8+ messages in thread
From: Mika Westerberg @ 2016-10-21 13:06 UTC (permalink / raw)
To: Jacek Anaszewski
Cc: Hui Chun Ong, rpurdie, linux-leds, jonathan.hearn,
julia.cartwright, Brad Mouring, Rafael J. Wysocki
On Fri, Oct 21, 2016 at 02:56:15PM +0200, Jacek Anaszewski wrote:
> 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@ni.com>
> > Signed-off-by: Brad Mouring <brad.mouring@ni.com>
> > ---
> > 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 = {
I think you want to have a platform driver here instead.
The ACPI core will then populate automatically resources and you do not
need to evaluate and parse _CRS yourself.
> > + .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@ni.com>");
> > +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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] leds: Add user LED driver for NIC78bx device
2016-10-21 13:06 ` Mika Westerberg
@ 2016-10-21 21:11 ` Rafael J. Wysocki
2016-10-25 10:33 ` Hui Chun Ong
0 siblings, 1 reply; 8+ messages in thread
From: Rafael J. Wysocki @ 2016-10-21 21:11 UTC (permalink / raw)
To: Mika Westerberg, Hui Chun Ong
Cc: Jacek Anaszewski, rpurdie, linux-leds, jonathan.hearn,
julia.cartwright, Brad Mouring
On Friday, October 21, 2016 04:06:04 PM Mika Westerberg wrote:
> On Fri, Oct 21, 2016 at 02:56:15PM +0200, Jacek Anaszewski wrote:
> > 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@ni.com>
> > > Signed-off-by: Brad Mouring <brad.mouring@ni.com>
> > > ---
> > > 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 = {
>
> I think you want to have a platform driver here instead.
Ah, thanks for the heads-up Mika!
As a rule, struct acpi_driver can only be used by things in drivers/acpi/.
If you think you need it *anyway*, the patch must be CCed to linux-acpi and
it is unlikely to be approved unless you have a very good reason to use
struct acpi_driver and very convincing arguments to support that.
Thanks,
Rafael
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] leds: Add user LED driver for NIC78bx device
2016-10-21 12:56 ` Jacek Anaszewski
2016-10-21 13:06 ` Mika Westerberg
@ 2016-10-21 21:33 ` Julia Cartwright
2016-10-23 14:12 ` Jacek Anaszewski
1 sibling, 1 reply; 8+ messages in thread
From: Julia Cartwright @ 2016-10-21 21:33 UTC (permalink / raw)
To: Jacek Anaszewski
Cc: Hui Chun Ong, rpurdie, linux-leds, jonathan.hearn, Brad Mouring,
Mika Westerberg, Rafael J. Wysocki
On Fri, Oct 21, 2016 at 02:56:15PM +0200, Jacek Anaszewski wrote:
> 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@ni.com>
> > Signed-off-by: Brad Mouring <brad.mouring@ni.com>
[..]
>
> > +
> > +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.
Could you explain why spin_lock_irqsave() is appropriate here? This
device doesn't have an interrupt to be synchronized with. I could
understand spin_lock()/spin_unlock(), maybe, but why disable interrupts?
Is what you're saying that the set/get brightness calls can be re-entered
by LED consumers in hardirq context?
The systems this driver is deployed to is much more sensitive to RT
execution latency than anything else, which is why I take pause to
unnecessary interrupt mask twiddling/preemption disabling :).
Julia
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] leds: Add user LED driver for NIC78bx device
2016-10-21 21:33 ` Julia Cartwright
@ 2016-10-23 14:12 ` Jacek Anaszewski
2016-10-25 10:07 ` Hui Chun Ong
0 siblings, 1 reply; 8+ messages in thread
From: Jacek Anaszewski @ 2016-10-23 14:12 UTC (permalink / raw)
To: Julia Cartwright, Jacek Anaszewski
Cc: Hui Chun Ong, rpurdie, linux-leds, jonathan.hearn, Brad Mouring,
Mika Westerberg, Rafael J. Wysocki
Hi Julia,
On 10/21/2016 11:33 PM, Julia Cartwright wrote:
> On Fri, Oct 21, 2016 at 02:56:15PM +0200, Jacek Anaszewski wrote:
>> 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@ni.com>
>>> Signed-off-by: Brad Mouring <brad.mouring@ni.com>
> [..]
>>
>>> +
>>> +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.
>
> Could you explain why spin_lock_irqsave() is appropriate here? This
> device doesn't have an interrupt to be synchronized with. I could
> understand spin_lock()/spin_unlock(), maybe, but why disable interrupts?
brightness_set op can be called from atomic context e.g. in case of
timer trigger. The code that runs in the interrupt context cannot sleep
due to reasons explained in [0], whereas mutex can sleep on contention.
spin_lock() is appropriate only in a hard irq handler.
> Is what you're saying that the set/get brightness calls can be re-entered
> by LED consumers in hardirq context?
>
> The systems this driver is deployed to is much more sensitive to RT
> execution latency than anything else, which is why I take pause to
> unnecessary interrupt mask twiddling/preemption disabling :).
[0] http://www.makelinux.net/ldd3/chp-5-sect-5
--
Best regards,
Jacek Anaszewski
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] leds: Add user LED driver for NIC78bx device
2016-10-23 14:12 ` Jacek Anaszewski
@ 2016-10-25 10:07 ` Hui Chun Ong
0 siblings, 0 replies; 8+ messages in thread
From: Hui Chun Ong @ 2016-10-25 10:07 UTC (permalink / raw)
To: Julia Cartwright, j.anaszewski, jacek.anaszewski
Cc: rpurdie, mika.westerberg, linux-leds, rjw, Jonathan Hearn, Brad Mouring
Hi Jacek,
On Sun, 2016-10-23 at 16:12 +0200, Jacek Anaszewski wrote:
> Hi Julia,
>
> On 10/21/2016 11:33 PM, Julia Cartwright wrote:
> >
> > On Fri, Oct 21, 2016 at 02:56:15PM +0200, Jacek Anaszewski wrote:
> > >
> > > 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@ni.com>
> > > > Signed-off-by: Brad Mouring <brad.mouring@ni.com>
> > [..]
> > >
> > >
> > > >
> > > > +
> > > > +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.
> > Could you explain why spin_lock_irqsave() is appropriate
> > here? This
> > device doesn't have an interrupt to be synchronized with. I could
> > understand spin_lock()/spin_unlock(), maybe, but why disable
> > interrupts?
> brightness_set op can be called from atomic context e.g. in case of
> timer trigger. The code that runs in the interrupt context cannot
> sleep
> due to reasons explained in [0], whereas mutex can sleep on
> contention.
>
> spin_lock() is appropriate only in a hard irq handler.
>
> >
> > Is what you're saying that the set/get brightness calls can be re-
> > entered
> > by LED consumers in hardirq context?
> >
> > The systems this driver is deployed to is much more sensitive to RT
> > execution latency than anything else, which is why I take pause to
> > unnecessary interrupt mask twiddling/preemption disabling :).
> [0] http://www.makelinux.net/ldd3/chp-5-sect-5
>
I'm update the driver to use spin_lock_irqsave. Thanks for the
feedback.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] leds: Add user LED driver for NIC78bx device
2016-10-21 21:11 ` Rafael J. Wysocki
@ 2016-10-25 10:33 ` Hui Chun Ong
0 siblings, 0 replies; 8+ messages in thread
From: Hui Chun Ong @ 2016-10-25 10:33 UTC (permalink / raw)
To: mika.westerberg, rjw
Cc: rpurdie, Julia Cartwright, j.anaszewski, linux-leds,
Jonathan Hearn, Brad Mouring
On Fri, 2016-10-21 at 23:11 +0200, Rafael J. Wysocki wrote:
> On Friday, October 21, 2016 04:06:04 PM Mika Westerberg wrote:
> >
> > On Fri, Oct 21, 2016 at 02:56:15PM +0200, Jacek Anaszewski wrote:
> > >
> > > 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@ni.com>
> > > > Signed-off-by: Brad Mouring <brad.mouring@ni.com>
> > > > ---
> > > > 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 = {
> > I think you want to have a platform driver here instead.
> Ah, thanks for the heads-up Mika!
>
> As a rule, struct acpi_driver can only be used by things in
> drivers/acpi/.
>
> If you think you need it *anyway*, the patch must be CCed to linux-
> acpi and
> it is unlikely to be approved unless you have a very good reason to
> use
> struct acpi_driver and very convincing arguments to support that.
>
> Thanks,
> Rafael
>
I'll explore using struct platform_driver. Thanks for your review
feedback.
rgds
Hui Chun
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-10-25 14:09 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <CGME20161021063441eucas1p221cdc9bd704f03ad367df8cf59fa35e6@eucas1p2.samsung.com>
2016-10-21 6:33 ` [PATCH] leds: Add user LED driver for NIC78bx device Hui Chun Ong
2016-10-21 12:56 ` Jacek Anaszewski
2016-10-21 13:06 ` Mika Westerberg
2016-10-21 21:11 ` Rafael J. Wysocki
2016-10-25 10:33 ` Hui Chun Ong
2016-10-21 21:33 ` Julia Cartwright
2016-10-23 14:12 ` Jacek Anaszewski
2016-10-25 10:07 ` Hui Chun Ong
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.