* [PATCH v2] leds: Add user LED driver for NIC78bx device @ 2016-10-27 11:02 ` Hui Chun Ong 2016-10-28 10:08 ` Jacek Anaszewski 0 siblings, 1 reply; 9+ messages in thread From: Hui Chun Ong @ 2016-10-27 11:02 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> --- v1: Update code to use spinlock. Change from acpi_driver to platform_driver. Create struct ni78bx_led_data to aggregate static variables. --- drivers/leds/Kconfig | 11 +++ drivers/leds/Makefile | 1 + drivers/leds/leds-ni78bx.c | 209 +++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 221 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..c5682ee --- /dev/null +++ b/drivers/leds/leds-ni78bx.c @@ -0,0 +1,209 @@ +/* + * 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/platform_device.h> +#include <linux/spinlock.h> + +#define NIC78BX_USER1_LED_MASK 0x3 +#define NIC78BX_USER1_GREEN_LED BIT(0) +#define NIC78BX_USER1_YELLOW_LED BIT(1) + +#define NIC78BX_USER2_LED_MASK 0xC +#define NIC78BX_USER2_GREEN_LED BIT(2) +#define NIC78BX_USER2_YELLOW_LED BIT(3) + +#define NIC78BX_LOCK_REG_OFFSET 1 +#define NIC78BX_LOCK_VALUE 0xA5 +#define NIC78BX_UNLOCK_VALUE 0x5A + +#define USER_LED_IO_SIZE 2 + +struct ni78bx_led_data { + u16 io_base; + spinlock_t lock; + struct platform_device *pdev; +}; + +struct ni78bx_led { + u8 bit; + u8 mask; + struct ni78bx_led_data *data; + 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); + unsigned long flags; + u8 value; + + spin_lock_irqsave(&nled->data->lock, flags); + value = inb(nled->data->io_base); + + if (brightness) { + value &= ~nled->mask; + value |= nled->bit; + } else { + value &= ~nled->bit; + } + + outb(value, nled->data->io_base); + spin_unlock_irqrestore(&nled->data->lock, flags); +} + +static enum led_brightness ni78bx_brightness_get(struct led_classdev *cdev) +{ + struct ni78bx_led *nled = to_ni78bx_led(cdev); + unsigned long flags; + u8 value; + + spin_lock_irqsave(&nled->data->lock, flags); + value = inb(nled->data->io_base); + spin_unlock_irqrestore(&nled->data->lock, flags); + + return (value & nled->bit) ? 1 : LED_OFF; +} + +static struct ni78bx_led ni78bx_leds[] = { + { + .bit = NIC78BX_USER1_GREEN_LED, + .mask = NIC78BX_USER1_LED_MASK, + .cdev = { + .name = "nilrt:green:user1", + .max_brightness = 1, + .brightness_set = ni78bx_brightness_set, + .brightness_get = ni78bx_brightness_get, + } + }, + { + .bit = NIC78BX_USER1_YELLOW_LED, + .mask = NIC78BX_USER1_LED_MASK, + .cdev = { + .name = "nilrt:yellow:user1", + .max_brightness = 1, + .brightness_set = ni78bx_brightness_set, + .brightness_get = ni78bx_brightness_get, + } + }, + { + .bit = NIC78BX_USER2_GREEN_LED, + .mask = NIC78BX_USER2_LED_MASK, + .cdev = { + .name = "nilrt:green:user2", + .max_brightness = 1, + .brightness_set = ni78bx_brightness_set, + .brightness_get = ni78bx_brightness_get, + } + }, + { + .bit = NIC78BX_USER2_YELLOW_LED, + .mask = NIC78BX_USER2_LED_MASK, + .cdev = { + .name = "nilrt:yellow:user2", + .max_brightness = 1, + .brightness_set = ni78bx_brightness_set, + .brightness_get = ni78bx_brightness_get, + } + } +}; + +static int ni78bx_remove(struct platform_device *pdev) +{ + struct ni78bx_led_data *led_data = platform_get_drvdata(pdev); + + /* Lock LED register */ + outb(NIC78BX_LOCK_VALUE, + led_data->io_base + NIC78BX_LOCK_REG_OFFSET); + + return 0; +} + +static int ni78bx_add(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct ni78bx_led_data *led_data; + struct resource *io_rc; + int ret, i; + + led_data = devm_kzalloc(dev, sizeof(*led_data), GFP_KERNEL); + if (!led_data) + return -ENOMEM; + + led_data->pdev = pdev; + platform_set_drvdata(pdev, led_data); + + io_rc = platform_get_resource(pdev, IORESOURCE_IO, 0); + if (!io_rc) { + dev_err(dev, "missing IO resources\n"); + return -EINVAL; + } + + if (resource_size(io_rc) < USER_LED_IO_SIZE) { + dev_err(dev, "IO region too small\n"); + return -EINVAL; + } + + if (!devm_request_region(dev, io_rc->start, resource_size(io_rc), + KBUILD_MODNAME)) { + dev_err(dev, "failed to get IO region\n"); + return -EBUSY; + } + + led_data->io_base = io_rc->start; + spin_lock_init(&led_data->lock); + + for (i = 0; i < ARRAY_SIZE(ni78bx_leds); i++) { + ni78bx_leds[i].data = led_data; + + ret = devm_led_classdev_register(dev, &ni78bx_leds[i].cdev); + if (ret) + return ret; + } + + /* Unlock LED register */ + outb(NIC78BX_UNLOCK_VALUE, + led_data->io_base + NIC78BX_LOCK_REG_OFFSET); + + return ret; +} + +static const struct acpi_device_id led_device_ids[] = { + {"NIC78B3", 0}, + {"", 0}, +}; +MODULE_DEVICE_TABLE(acpi, led_device_ids); + +static struct platform_driver led_driver = { + .probe = ni78bx_add, + .remove = ni78bx_remove, + .driver = { + .name = KBUILD_MODNAME, + .acpi_match_table = ACPI_PTR(led_device_ids), + }, +}; + +module_platform_driver(led_driver); + +MODULE_DESCRIPTION("National Instruments PXI User LEDs driver"); +MODULE_AUTHOR("Hui Chun Ong <hui.chun.ong@ni.com>"); +MODULE_LICENSE("GPL"); -- 2.7.4 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] leds: Add user LED driver for NIC78bx device 2016-10-27 11:02 ` [PATCH v2] leds: Add user LED driver for NIC78bx device Hui Chun Ong @ 2016-10-28 10:08 ` Jacek Anaszewski 2016-10-31 8:47 ` Mika Westerberg 2016-11-01 8:23 ` Hui Chun Ong 0 siblings, 2 replies; 9+ messages in thread From: Jacek Anaszewski @ 2016-10-28 10:08 UTC (permalink / raw) To: Hui Chun Ong, rpurdie Cc: linux-leds, jonathan.hearn, julia.cartwright, Brad Mouring, Rafael J. Wysocki, Mika Westerberg Hi Hui, Thanks for the update. On 10/27/2016 01:02 PM, 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> > --- > v1: Update code to use spinlock. > Change from acpi_driver to platform_driver. > Create struct ni78bx_led_data to aggregate static variables. > --- > drivers/leds/Kconfig | 11 +++ > drivers/leds/Makefile | 1 + > drivers/leds/leds-ni78bx.c | 209 +++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 221 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..c5682ee > --- /dev/null > +++ b/drivers/leds/leds-ni78bx.c > @@ -0,0 +1,209 @@ > +/* > + * 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/platform_device.h> > +#include <linux/spinlock.h> > + > +#define NIC78BX_USER1_LED_MASK 0x3 > +#define NIC78BX_USER1_GREEN_LED BIT(0) > +#define NIC78BX_USER1_YELLOW_LED BIT(1) > + > +#define NIC78BX_USER2_LED_MASK 0xC > +#define NIC78BX_USER2_GREEN_LED BIT(2) > +#define NIC78BX_USER2_YELLOW_LED BIT(3) > + > +#define NIC78BX_LOCK_REG_OFFSET 1 > +#define NIC78BX_LOCK_VALUE 0xA5 > +#define NIC78BX_UNLOCK_VALUE 0x5A > + > +#define USER_LED_IO_SIZE 2 This macro also requires prefix. > + > +struct ni78bx_led_data { > + u16 io_base; > + spinlock_t lock; > + struct platform_device *pdev; > +}; > + > +struct ni78bx_led { > + u8 bit; > + u8 mask; > + struct ni78bx_led_data *data; > + 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); > + unsigned long flags; > + u8 value; > + > + spin_lock_irqsave(&nled->data->lock, flags); > + value = inb(nled->data->io_base); > + > + if (brightness) { > + value &= ~nled->mask; > + value |= nled->bit; > + } else { > + value &= ~nled->bit; > + } > + > + outb(value, nled->data->io_base); > + spin_unlock_irqrestore(&nled->data->lock, flags); > +} > + > +static enum led_brightness ni78bx_brightness_get(struct led_classdev *cdev) > +{ > + struct ni78bx_led *nled = to_ni78bx_led(cdev); > + unsigned long flags; > + u8 value; > + > + spin_lock_irqsave(&nled->data->lock, flags); > + value = inb(nled->data->io_base); > + spin_unlock_irqrestore(&nled->data->lock, flags); > + > + return (value & nled->bit) ? 1 : LED_OFF; > +} > + > +static struct ni78bx_led ni78bx_leds[] = { > + { > + .bit = NIC78BX_USER1_GREEN_LED, > + .mask = NIC78BX_USER1_LED_MASK, > + .cdev = { > + .name = "nilrt:green:user1", Why nilrt prefix? There is some discrepancy in the naming allover this patch. You have: - NIC78bx in the commit title - ni78bx in the function prefixes - nilrt in the LED class device names Please make it uniform. What actually the device name is? > + .max_brightness = 1, > + .brightness_set = ni78bx_brightness_set, > + .brightness_get = ni78bx_brightness_get, > + } > + }, > + { > + .bit = NIC78BX_USER1_YELLOW_LED, > + .mask = NIC78BX_USER1_LED_MASK, > + .cdev = { > + .name = "nilrt:yellow:user1", > + .max_brightness = 1, > + .brightness_set = ni78bx_brightness_set, > + .brightness_get = ni78bx_brightness_get, > + } > + }, > + { > + .bit = NIC78BX_USER2_GREEN_LED, > + .mask = NIC78BX_USER2_LED_MASK, > + .cdev = { > + .name = "nilrt:green:user2", > + .max_brightness = 1, > + .brightness_set = ni78bx_brightness_set, > + .brightness_get = ni78bx_brightness_get, > + } > + }, > + { > + .bit = NIC78BX_USER2_YELLOW_LED, > + .mask = NIC78BX_USER2_LED_MASK, > + .cdev = { > + .name = "nilrt:yellow:user2", > + .max_brightness = 1, > + .brightness_set = ni78bx_brightness_set, > + .brightness_get = ni78bx_brightness_get, > + } > + } > +}; > + > +static int ni78bx_remove(struct platform_device *pdev) > +{ > + struct ni78bx_led_data *led_data = platform_get_drvdata(pdev); > + > + /* Lock LED register */ > + outb(NIC78BX_LOCK_VALUE, > + led_data->io_base + NIC78BX_LOCK_REG_OFFSET); > + > + return 0; > +} > + > +static int ni78bx_add(struct platform_device *pdev) > +{ s/add/probe/ Please also put ni78bx_remove() after this function. > + struct device *dev = &pdev->dev; > + struct ni78bx_led_data *led_data; > + struct resource *io_rc; > + int ret, i; > + > + led_data = devm_kzalloc(dev, sizeof(*led_data), GFP_KERNEL); > + if (!led_data) > + return -ENOMEM; > + > + led_data->pdev = pdev; > + platform_set_drvdata(pdev, led_data); > + > + io_rc = platform_get_resource(pdev, IORESOURCE_IO, 0); > + if (!io_rc) { > + dev_err(dev, "missing IO resources\n"); > + return -EINVAL; > + } > + > + if (resource_size(io_rc) < USER_LED_IO_SIZE) { > + dev_err(dev, "IO region too small\n"); > + return -EINVAL; > + } > + > + if (!devm_request_region(dev, io_rc->start, resource_size(io_rc), > + KBUILD_MODNAME)) { > + dev_err(dev, "failed to get IO region\n"); > + return -EBUSY; > + } > + > + led_data->io_base = io_rc->start; > + spin_lock_init(&led_data->lock); > + > + for (i = 0; i < ARRAY_SIZE(ni78bx_leds); i++) { > + ni78bx_leds[i].data = led_data; > + > + ret = devm_led_classdev_register(dev, &ni78bx_leds[i].cdev); > + if (ret) > + return ret; > + } > + > + /* Unlock LED register */ > + outb(NIC78BX_UNLOCK_VALUE, > + led_data->io_base + NIC78BX_LOCK_REG_OFFSET); > + > + return ret; > +} > + > +static const struct acpi_device_id led_device_ids[] = { > + {"NIC78B3", 0}, > + {"", 0}, > +}; > +MODULE_DEVICE_TABLE(acpi, led_device_ids); > + > +static struct platform_driver led_driver = { > + .probe = ni78bx_add, > + .remove = ni78bx_remove, > + .driver = { > + .name = KBUILD_MODNAME, > + .acpi_match_table = ACPI_PTR(led_device_ids), Please CC ACPI maintainers always if you include acpi.h Cc Rafael and Mika. > + }, > +}; > + > +module_platform_driver(led_driver); > + > +MODULE_DESCRIPTION("National Instruments PXI User LEDs driver"); > +MODULE_AUTHOR("Hui Chun Ong <hui.chun.ong@ni.com>"); > +MODULE_LICENSE("GPL"); > -- > 2.7.4 > > > > -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] leds: Add user LED driver for NIC78bx device 2016-10-28 10:08 ` Jacek Anaszewski @ 2016-10-31 8:47 ` Mika Westerberg 2016-11-02 8:22 ` Jacek Anaszewski 2016-11-01 8:23 ` Hui Chun Ong 1 sibling, 1 reply; 9+ messages in thread From: Mika Westerberg @ 2016-10-31 8:47 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 28, 2016 at 12:08:57PM +0200, Jacek Anaszewski wrote: > > +static struct platform_driver led_driver = { > > + .probe = ni78bx_add, > > + .remove = ni78bx_remove, > > + .driver = { > > + .name = KBUILD_MODNAME, > > + .acpi_match_table = ACPI_PTR(led_device_ids), > > Please CC ACPI maintainers always if you include acpi.h > Cc Rafael and Mika. >From ACPI perspective this driver looks fine to me now. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] leds: Add user LED driver for NIC78bx device 2016-10-31 8:47 ` Mika Westerberg @ 2016-11-02 8:22 ` Jacek Anaszewski 2016-11-02 10:00 ` Mika Westerberg 0 siblings, 1 reply; 9+ messages in thread From: Jacek Anaszewski @ 2016-11-02 8:22 UTC (permalink / raw) To: Mika Westerberg Cc: Hui Chun Ong, rpurdie, linux-leds, jonathan.hearn, julia.cartwright, Brad Mouring, Rafael J. Wysocki Hi Mika, On 10/31/2016 09:47 AM, Mika Westerberg wrote: > On Fri, Oct 28, 2016 at 12:08:57PM +0200, Jacek Anaszewski wrote: >>> +static struct platform_driver led_driver = { >>> + .probe = ni78bx_add, >>> + .remove = ni78bx_remove, >>> + .driver = { >>> + .name = KBUILD_MODNAME, >>> + .acpi_match_table = ACPI_PTR(led_device_ids), >> >> Please CC ACPI maintainers always if you include acpi.h >> Cc Rafael and Mika. > >>From ACPI perspective this driver looks fine to me now. Thanks for the review. Can I add your Acked-by then? -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] leds: Add user LED driver for NIC78bx device 2016-11-02 8:22 ` Jacek Anaszewski @ 2016-11-02 10:00 ` Mika Westerberg 0 siblings, 0 replies; 9+ messages in thread From: Mika Westerberg @ 2016-11-02 10:00 UTC (permalink / raw) To: Jacek Anaszewski Cc: Hui Chun Ong, rpurdie, linux-leds, jonathan.hearn, julia.cartwright, Brad Mouring, Rafael J. Wysocki On Wed, Nov 02, 2016 at 09:22:44AM +0100, Jacek Anaszewski wrote: > Hi Mika, > > On 10/31/2016 09:47 AM, Mika Westerberg wrote: > > On Fri, Oct 28, 2016 at 12:08:57PM +0200, Jacek Anaszewski wrote: > > > > +static struct platform_driver led_driver = { > > > > + .probe = ni78bx_add, > > > > + .remove = ni78bx_remove, > > > > + .driver = { > > > > + .name = KBUILD_MODNAME, > > > > + .acpi_match_table = ACPI_PTR(led_device_ids), > > > > > > Please CC ACPI maintainers always if you include acpi.h > > > Cc Rafael and Mika. > > > > > From ACPI perspective this driver looks fine to me now. > > Thanks for the review. Can I add your Acked-by then? Sure, Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com> ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] leds: Add user LED driver for NIC78bx device 2016-10-28 10:08 ` Jacek Anaszewski 2016-10-31 8:47 ` Mika Westerberg @ 2016-11-01 8:23 ` Hui Chun Ong 2016-11-01 17:48 ` Jacek Anaszewski 1 sibling, 1 reply; 9+ messages in thread From: Hui Chun Ong @ 2016-11-01 8:23 UTC (permalink / raw) To: rpurdie, j.anaszewski Cc: mika.westerberg, Julia Cartwright, linux-leds, rjw, Jonathan Hearn, Brad Mouring Hi Jacek, On Fri, 2016-10-28 at 12:08 +0200, Jacek Anaszewski wrote: > Hi Hui, > > Thanks for the update. > > On 10/27/2016 01:02 PM, 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> > > --- > > v1: Update code to use spinlock. > > Change from acpi_driver to platform_driver. > > Create struct ni78bx_led_data to aggregate static variables. > > --- > > drivers/leds/Kconfig | 11 +++ > > drivers/leds/Makefile | 1 + > > drivers/leds/leds-ni78bx.c | 209 +++++++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 221 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..c5682ee > > --- /dev/null > > +++ b/drivers/leds/leds-ni78bx.c > > @@ -0,0 +1,209 @@ > > +/* > > + * 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 > > +#include > > +#include > > +#include > > +#include > > + > > +#define NIC78BX_USER1_LED_MASK 0x3 > > +#define NIC78BX_USER1_GREEN_LED BIT(0) > > +#define NIC78BX_USER1_YELLOW_LED BIT(1) > > + > > +#define NIC78BX_USER2_LED_MASK 0xC > > +#define NIC78BX_USER2_GREEN_LED BIT(2) > > +#define NIC78BX_USER2_YELLOW_LED BIT(3) > > + > > +#define NIC78BX_LOCK_REG_OFFSET 1 > > +#define NIC78BX_LOCK_VALUE 0xA5 > > +#define NIC78BX_UNLOCK_VALUE 0x5A > > + > > +#define USER_LED_IO_SIZE 2 > This macro also requires prefix. > > > > > + > > +struct ni78bx_led_data { > > + u16 io_base; > > + spinlock_t lock; > > + struct platform_device *pdev; > > +}; > > + > > +struct ni78bx_led { > > + u8 bit; > > + u8 mask; > > + struct ni78bx_led_data *data; > > + 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); > > + unsigned long flags; > > + u8 value; > > + > > + spin_lock_irqsave(&nled->data->lock, flags); > > + value = inb(nled->data->io_base); > > + > > + if (brightness) { > > + value &= ~nled->mask; > > + value |= nled->bit; > > + } else { > > + value &= ~nled->bit; > > + } > > + > > + outb(value, nled->data->io_base); > > + spin_unlock_irqrestore(&nled->data->lock, flags); > > +} > > + > > +static enum led_brightness ni78bx_brightness_get(struct led_classdev *cdev) > > +{ > > + struct ni78bx_led *nled = to_ni78bx_led(cdev); > > + unsigned long flags; > > + u8 value; > > + > > + spin_lock_irqsave(&nled->data->lock, flags); > > + value = inb(nled->data->io_base); > > + spin_unlock_irqrestore(&nled->data->lock, flags); > > + > > + return (value & nled->bit) ? 1 : LED_OFF; > > +} > > + > > +static struct ni78bx_led ni78bx_leds[] = { > > + { > > + .bit = NIC78BX_USER1_GREEN_LED, > > + .mask = NIC78BX_USER1_LED_MASK, > > + .cdev = { > > + .name = "nilrt:green:user1", > Why nilrt prefix? There is some discrepancy in the naming allover > this patch. You have: > > - NIC78bx in the commit title > - ni78bx in the function prefixes > - nilrt in the LED class device names > > Please make it uniform. What actually the device name is? > The device name is NIC78bx. I'll update all ni78bx references to nic78bx. However, for the LED class device name, I'm hoping to use a more generic name and not device specific name since essentially it's just a user controllable LEDs. Something like "niled:green:user1" or "green:user1". > > > > + .max_brightness = 1, > > + .brightness_set = ni78bx_brightness_set, > > + .brightness_get = ni78bx_brightness_get, > > + } > > + }, > > + { > > + .bit = NIC78BX_USER1_YELLOW_LED, > > + .mask = NIC78BX_USER1_LED_MASK, > > + .cdev = { > > + .name = "nilrt:yellow:user1", > > + .max_brightness = 1, > > + .brightness_set = ni78bx_brightness_set, > > + .brightness_get = ni78bx_brightness_get, > > + } > > + }, > > + { > > + .bit = NIC78BX_USER2_GREEN_LED, > > + .mask = NIC78BX_USER2_LED_MASK, > > + .cdev = { > > + .name = "nilrt:green:user2", > > + .max_brightness = 1, > > + .brightness_set = ni78bx_brightness_set, > > + .brightness_get = ni78bx_brightness_get, > > + } > > + }, > > + { > > + .bit = NIC78BX_USER2_YELLOW_LED, > > + .mask = NIC78BX_USER2_LED_MASK, > > + .cdev = { > > + .name = "nilrt:yellow:user2", > > + .max_brightness = 1, > > + .brightness_set = ni78bx_brightness_set, > > + .brightness_get = ni78bx_brightness_get, > > + } > > + } > > +}; > > + > > +static int ni78bx_remove(struct platform_device *pdev) > > +{ > > + struct ni78bx_led_data *led_data = platform_get_drvdata(pdev); > > + > > + /* Lock LED register */ > > + outb(NIC78BX_LOCK_VALUE, > > + led_data->io_base + NIC78BX_LOCK_REG_OFFSET); > > + > > + return 0; > > +} > > + > > +static int ni78bx_add(struct platform_device *pdev) > > +{ > s/add/probe/ Will update to ni78bx_probe > > Please also put ni78bx_remove() after this function. > > > > > + struct device *dev = &pdev->dev; > > + struct ni78bx_led_data *led_data; > > + struct resource *io_rc; > > + int ret, i; > > + > > + led_data = devm_kzalloc(dev, sizeof(*led_data), GFP_KERNEL); > > + if (!led_data) > > + return -ENOMEM; > > + > > + led_data->pdev = pdev; > > + platform_set_drvdata(pdev, led_data); > > + > > + io_rc = platform_get_resource(pdev, IORESOURCE_IO, 0); > > + if (!io_rc) { > > + dev_err(dev, "missing IO resources\n"); > > + return -EINVAL; > > + } > > + > > + if (resource_size(io_rc) < USER_LED_IO_SIZE) { > > + dev_err(dev, "IO region too small\n"); > > + return -EINVAL; > > + } > > + > > + if (!devm_request_region(dev, io_rc->start, resource_size(io_rc), > > + KBUILD_MODNAME)) { > > + dev_err(dev, "failed to get IO region\n"); > > + return -EBUSY; > > + } > > + > > + led_data->io_base = io_rc->start; > > + spin_lock_init(&led_data->lock); > > + > > + for (i = 0; i < ARRAY_SIZE(ni78bx_leds); i++) { > > + ni78bx_leds[i].data = led_data; > > + > > + ret = devm_led_classdev_register(dev, &ni78bx_leds[i].cdev); > > + if (ret) > > + return ret; > > + } > > + > > + /* Unlock LED register */ > > + outb(NIC78BX_UNLOCK_VALUE, > > + led_data->io_base + NIC78BX_LOCK_REG_OFFSET); > > + > > + return ret; > > +} > > + > > +static const struct acpi_device_id led_device_ids[] = { > > + {"NIC78B3", 0}, > > + {"", 0}, > > +}; > > +MODULE_DEVICE_TABLE(acpi, led_device_ids); > > + > > +static struct platform_driver led_driver = { > > + .probe = ni78bx_add, > > + .remove = ni78bx_remove, > > + .driver = { > > + .name = KBUILD_MODNAME, > > + .acpi_match_table = ACPI_PTR(led_device_ids), > Please CC ACPI maintainers always if you include acpi.h > Cc Rafael and Mika. > > > > > + }, > > +}; > > + > > +module_platform_driver(led_driver); > > + > > +MODULE_DESCRIPTION("National Instruments PXI User LEDs driver"); > > +MODULE_AUTHOR("Hui Chun Ong <hui.chun.ong@ni.com>"); > > +MODULE_LICENSE("GPL"); > > -- > > 2.7.4 > > > > > > > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] leds: Add user LED driver for NIC78bx device 2016-11-01 8:23 ` Hui Chun Ong @ 2016-11-01 17:48 ` Jacek Anaszewski 2016-11-03 14:43 ` Hui Chun Ong 0 siblings, 1 reply; 9+ messages in thread From: Jacek Anaszewski @ 2016-11-01 17:48 UTC (permalink / raw) To: Hui Chun Ong, rpurdie, j.anaszewski Cc: mika.westerberg, Julia Cartwright, linux-leds, rjw, Jonathan Hearn, Brad Mouring Hi Hui. On 11/01/2016 09:23 AM, Hui Chun Ong wrote: > Hi Jacek, > > On Fri, 2016-10-28 at 12:08 +0200, Jacek Anaszewski wrote: >> Hi Hui, >> >> Thanks for the update. >> >> On 10/27/2016 01:02 PM, 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> >>> --- >>> v1: Update code to use spinlock. >>> Change from acpi_driver to platform_driver. >>> Create struct ni78bx_led_data to aggregate static variables. >>> --- >>> drivers/leds/Kconfig | 11 +++ >>> drivers/leds/Makefile | 1 + >>> drivers/leds/leds-ni78bx.c | 209 +++++++++++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 221 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..c5682ee >>> --- /dev/null >>> +++ b/drivers/leds/leds-ni78bx.c >>> @@ -0,0 +1,209 @@ >>> +/* >>> + * 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 >>> +#include >>> +#include >>> +#include >>> +#include >>> + >>> +#define NIC78BX_USER1_LED_MASK 0x3 >>> +#define NIC78BX_USER1_GREEN_LED BIT(0) >>> +#define NIC78BX_USER1_YELLOW_LED BIT(1) >>> + >>> +#define NIC78BX_USER2_LED_MASK 0xC >>> +#define NIC78BX_USER2_GREEN_LED BIT(2) >>> +#define NIC78BX_USER2_YELLOW_LED BIT(3) >>> + >>> +#define NIC78BX_LOCK_REG_OFFSET 1 >>> +#define NIC78BX_LOCK_VALUE 0xA5 >>> +#define NIC78BX_UNLOCK_VALUE 0x5A >>> + >>> +#define USER_LED_IO_SIZE 2 >> This macro also requires prefix. >> >>> >>> + >>> +struct ni78bx_led_data { >>> + u16 io_base; >>> + spinlock_t lock; >>> + struct platform_device *pdev; >>> +}; >>> + >>> +struct ni78bx_led { >>> + u8 bit; >>> + u8 mask; >>> + struct ni78bx_led_data *data; >>> + 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); >>> + unsigned long flags; >>> + u8 value; >>> + >>> + spin_lock_irqsave(&nled->data->lock, flags); >>> + value = inb(nled->data->io_base); >>> + >>> + if (brightness) { >>> + value &= ~nled->mask; >>> + value |= nled->bit; >>> + } else { >>> + value &= ~nled->bit; >>> + } >>> + >>> + outb(value, nled->data->io_base); >>> + spin_unlock_irqrestore(&nled->data->lock, flags); >>> +} >>> + >>> +static enum led_brightness ni78bx_brightness_get(struct led_classdev *cdev) >>> +{ >>> + struct ni78bx_led *nled = to_ni78bx_led(cdev); >>> + unsigned long flags; >>> + u8 value; >>> + >>> + spin_lock_irqsave(&nled->data->lock, flags); >>> + value = inb(nled->data->io_base); >>> + spin_unlock_irqrestore(&nled->data->lock, flags); >>> + >>> + return (value & nled->bit) ? 1 : LED_OFF; >>> +} >>> + >>> +static struct ni78bx_led ni78bx_leds[] = { >>> + { >>> + .bit = NIC78BX_USER1_GREEN_LED, >>> + .mask = NIC78BX_USER1_LED_MASK, >>> + .cdev = { >>> + .name = "nilrt:green:user1", >> Why nilrt prefix? There is some discrepancy in the naming allover >> this patch. You have: >> >> - NIC78bx in the commit title >> - ni78bx in the function prefixes >> - nilrt in the LED class device names >> >> Please make it uniform. What actually the device name is? >> > The device name is NIC78bx. I'll update all ni78bx references to > nic78bx. However, for the LED class device name, I'm hoping to > use a more generic name and not device specific name since > essentially it's just a user controllable LEDs. Something like > "niled:green:user1" or "green:user1". LED class devince naming convention is <devicename:colour:function>. See "LED Device Naming" section in Documentation/leds/leds-class.txt. Please use nic78bx also for devicename segment. -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] leds: Add user LED driver for NIC78bx device 2016-11-01 17:48 ` Jacek Anaszewski @ 2016-11-03 14:43 ` Hui Chun Ong 2016-11-03 16:02 ` Jacek Anaszewski 0 siblings, 1 reply; 9+ messages in thread From: Hui Chun Ong @ 2016-11-03 14:43 UTC (permalink / raw) To: rpurdie, j.anaszewski, jacek.anaszewski Cc: mika.westerberg, Julia Cartwright, linux-leds, rjw, Jonathan Hearn, Brad Mouring Hi Jacek, On Tue, 2016-11-01 at 18:48 +0100, Jacek Anaszewski wrote: > Hi Hui. > > On 11/01/2016 09:23 AM, Hui Chun Ong wrote: > > > > Hi Jacek, > > > > On Fri, 2016-10-28 at 12:08 +0200, Jacek Anaszewski wrote: > > > > > > Hi Hui, > > > > > > Thanks for the update. > > > > > > On 10/27/2016 01:02 PM, 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> > > > > --- > > > > v1: Update code to use spinlock. > > > > Change from acpi_driver to platform_driver. > > > > Create struct ni78bx_led_data to aggregate static variables. > > > > --- > > > > drivers/leds/Kconfig | 11 +++ > > > > drivers/leds/Makefile | 1 + > > > > drivers/leds/leds-ni78bx.c | 209 +++++++++++++++++++++++++++++++++++++++++++++ > > > > 3 files changed, 221 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..c5682ee > > > > --- /dev/null > > > > +++ b/drivers/leds/leds-ni78bx.c > > > > @@ -0,0 +1,209 @@ > > > > +/* > > > > + * 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 > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > + > > > > +#define NIC78BX_USER1_LED_MASK 0x3 > > > > +#define NIC78BX_USER1_GREEN_LED BIT(0) > > > > +#define NIC78BX_USER1_YELLOW_LED BIT(1) > > > > + > > > > +#define NIC78BX_USER2_LED_MASK 0xC > > > > +#define NIC78BX_USER2_GREEN_LED BIT(2) > > > > +#define NIC78BX_USER2_YELLOW_LED BIT(3) > > > > + > > > > +#define NIC78BX_LOCK_REG_OFFSET 1 > > > > +#define NIC78BX_LOCK_VALUE 0xA5 > > > > +#define NIC78BX_UNLOCK_VALUE 0x5A > > > > + > > > > +#define USER_LED_IO_SIZE 2 > > > This macro also requires prefix. > > > > > > > > > > > > > > > + > > > > +struct ni78bx_led_data { > > > > + u16 io_base; > > > > + spinlock_t lock; > > > > + struct platform_device *pdev; > > > > +}; > > > > + > > > > +struct ni78bx_led { > > > > + u8 bit; > > > > + u8 mask; > > > > + struct ni78bx_led_data *data; > > > > + 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); > > > > + unsigned long flags; > > > > + u8 value; > > > > + > > > > + spin_lock_irqsave(&nled->data->lock, flags); > > > > + value = inb(nled->data->io_base); > > > > + > > > > + if (brightness) { > > > > + value &= ~nled->mask; > > > > + value |= nled->bit; > > > > + } else { > > > > + value &= ~nled->bit; > > > > + } > > > > + > > > > + outb(value, nled->data->io_base); > > > > + spin_unlock_irqrestore(&nled->data->lock, flags); > > > > +} > > > > + > > > > +static enum led_brightness ni78bx_brightness_get(struct led_classdev *cdev) > > > > +{ > > > > + struct ni78bx_led *nled = to_ni78bx_led(cdev); > > > > + unsigned long flags; > > > > + u8 value; > > > > + > > > > + spin_lock_irqsave(&nled->data->lock, flags); > > > > + value = inb(nled->data->io_base); > > > > + spin_unlock_irqrestore(&nled->data->lock, flags); > > > > + > > > > + return (value & nled->bit) ? 1 : LED_OFF; > > > > +} > > > > + > > > > +static struct ni78bx_led ni78bx_leds[] = { > > > > + { > > > > + .bit = NIC78BX_USER1_GREEN_LED, > > > > + .mask = NIC78BX_USER1_LED_MASK, > > > > + .cdev = { > > > > + .name = "nilrt:green:user1", > > > Why nilrt prefix? There is some discrepancy in the naming allover > > > this patch. You have: > > > > > > - NIC78bx in the commit title > > > - ni78bx in the function prefixes > > > - nilrt in the LED class device names > > > > > > Please make it uniform. What actually the device name is? > > > > > The device name is NIC78bx. I'll update all ni78bx references to > > nic78bx. However, for the LED class device name, I'm hoping to > > use a more generic name and not device specific name since > > essentially it's just a user controllable LEDs. Something like > > "niled:green:user1" or "green:user1". > LED class devince naming convention is . > See "LED Device Naming" section in Documentation/leds/leds-class.txt. > Please use nic78bx also for devicename segment. > At National Instruments we have a family of products that exposes LEDs control to the user. The backend LED control mechanism is hardware specific but at the front-end we want to provide a common interface to the userspace. Our product is guaranteed with a unique LED control mechanism for each product, thus, there is no risk for collision. We have done this for the past 20 years and have built application around a common interface on other OS platform which we hope to replicate in Linux. The "nilrt" prefix that we propose to use is our preferred common devicename for the family of products that support this feature as I described it above. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] leds: Add user LED driver for NIC78bx device 2016-11-03 14:43 ` Hui Chun Ong @ 2016-11-03 16:02 ` Jacek Anaszewski 0 siblings, 0 replies; 9+ messages in thread From: Jacek Anaszewski @ 2016-11-03 16:02 UTC (permalink / raw) To: Hui Chun Ong, rpurdie, jacek.anaszewski Cc: mika.westerberg, Julia Cartwright, linux-leds, rjw, Jonathan Hearn, Brad Mouring Hi Hui, On 11/03/2016 03:43 PM, Hui Chun Ong wrote: > Hi Jacek, > > On Tue, 2016-11-01 at 18:48 +0100, Jacek Anaszewski wrote: >> Hi Hui. >> >> On 11/01/2016 09:23 AM, Hui Chun Ong wrote: >>> >>> Hi Jacek, >>> >>> On Fri, 2016-10-28 at 12:08 +0200, Jacek Anaszewski wrote: >>>> >>>> Hi Hui, >>>> >>>> Thanks for the update. >>>> >>>> On 10/27/2016 01:02 PM, 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> >>>>> --- >>>>> v1: Update code to use spinlock. >>>>> Change from acpi_driver to platform_driver. >>>>> Create struct ni78bx_led_data to aggregate static variables. >>>>> --- >>>>> drivers/leds/Kconfig | 11 +++ >>>>> drivers/leds/Makefile | 1 + >>>>> drivers/leds/leds-ni78bx.c | 209 +++++++++++++++++++++++++++++++++++++++++++++ >>>>> 3 files changed, 221 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..c5682ee >>>>> --- /dev/null >>>>> +++ b/drivers/leds/leds-ni78bx.c >>>>> @@ -0,0 +1,209 @@ >>>>> +/* >>>>> + * 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 >>>>> +#include >>>>> +#include >>>>> +#include >>>>> +#include >>>>> + >>>>> +#define NIC78BX_USER1_LED_MASK 0x3 >>>>> +#define NIC78BX_USER1_GREEN_LED BIT(0) >>>>> +#define NIC78BX_USER1_YELLOW_LED BIT(1) >>>>> + >>>>> +#define NIC78BX_USER2_LED_MASK 0xC >>>>> +#define NIC78BX_USER2_GREEN_LED BIT(2) >>>>> +#define NIC78BX_USER2_YELLOW_LED BIT(3) >>>>> + >>>>> +#define NIC78BX_LOCK_REG_OFFSET 1 >>>>> +#define NIC78BX_LOCK_VALUE 0xA5 >>>>> +#define NIC78BX_UNLOCK_VALUE 0x5A >>>>> + >>>>> +#define USER_LED_IO_SIZE 2 >>>> This macro also requires prefix. >>>> >>>>> >>>>> >>>>> + >>>>> +struct ni78bx_led_data { >>>>> + u16 io_base; >>>>> + spinlock_t lock; >>>>> + struct platform_device *pdev; >>>>> +}; >>>>> + >>>>> +struct ni78bx_led { >>>>> + u8 bit; >>>>> + u8 mask; >>>>> + struct ni78bx_led_data *data; >>>>> + 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); >>>>> + unsigned long flags; >>>>> + u8 value; >>>>> + >>>>> + spin_lock_irqsave(&nled->data->lock, flags); >>>>> + value = inb(nled->data->io_base); >>>>> + >>>>> + if (brightness) { >>>>> + value &= ~nled->mask; >>>>> + value |= nled->bit; >>>>> + } else { >>>>> + value &= ~nled->bit; >>>>> + } >>>>> + >>>>> + outb(value, nled->data->io_base); >>>>> + spin_unlock_irqrestore(&nled->data->lock, flags); >>>>> +} >>>>> + >>>>> +static enum led_brightness ni78bx_brightness_get(struct led_classdev *cdev) >>>>> +{ >>>>> + struct ni78bx_led *nled = to_ni78bx_led(cdev); >>>>> + unsigned long flags; >>>>> + u8 value; >>>>> + >>>>> + spin_lock_irqsave(&nled->data->lock, flags); >>>>> + value = inb(nled->data->io_base); >>>>> + spin_unlock_irqrestore(&nled->data->lock, flags); >>>>> + >>>>> + return (value & nled->bit) ? 1 : LED_OFF; >>>>> +} >>>>> + >>>>> +static struct ni78bx_led ni78bx_leds[] = { >>>>> + { >>>>> + .bit = NIC78BX_USER1_GREEN_LED, >>>>> + .mask = NIC78BX_USER1_LED_MASK, >>>>> + .cdev = { >>>>> + .name = "nilrt:green:user1", >>>> Why nilrt prefix? There is some discrepancy in the naming allover >>>> this patch. You have: >>>> >>>> - NIC78bx in the commit title >>>> - ni78bx in the function prefixes >>>> - nilrt in the LED class device names >>>> >>>> Please make it uniform. What actually the device name is? >>>> >>> The device name is NIC78bx. I'll update all ni78bx references to >>> nic78bx. However, for the LED class device name, I'm hoping to >>> use a more generic name and not device specific name since >>> essentially it's just a user controllable LEDs. Something like >>> "niled:green:user1" or "green:user1". >> LED class devince naming convention is . >> See "LED Device Naming" section in Documentation/leds/leds-class.txt. >> Please use nic78bx also for devicename segment. >> > At National Instruments we have a family of products that exposes LEDs > control to the user. The backend LED control mechanism is hardware > specific but at the front-end we want to provide a common interface to > the userspace. > > Our product is guaranteed with a unique LED control mechanism for each > product, thus, there is no risk for collision. We have done this for > the past 20 years and have built application around a common interface > on other OS platform which we hope to replicate in Linux. > > The "nilrt" prefix that we propose to use is our preferred common > devicename for the family of products that support this feature as I > described it above. > Thanks for the explanation. Let's stay by "nilrt" then. -- Best regards, Jacek Anaszewski ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-11-03 16:02 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <CGME20161027110331epcas3p332bf735529a71e5c5efa66adf4312785@epcas3p3.samsung.com> 2016-10-27 11:02 ` [PATCH v2] leds: Add user LED driver for NIC78bx device Hui Chun Ong 2016-10-28 10:08 ` Jacek Anaszewski 2016-10-31 8:47 ` Mika Westerberg 2016-11-02 8:22 ` Jacek Anaszewski 2016-11-02 10:00 ` Mika Westerberg 2016-11-01 8:23 ` Hui Chun Ong 2016-11-01 17:48 ` Jacek Anaszewski 2016-11-03 14:43 ` Hui Chun Ong 2016-11-03 16:02 ` Jacek Anaszewski
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.