* [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 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
* 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
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.