[PATCH] leds: Add user LED driver for NIC78bx device

[PATCH] leds: Add user LED driver for NIC78bx device

[Hui Chun Ong]

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

Re: [PATCH] leds: Add user LED driver for NIC78bx device

[Jacek Anaszewski]

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

Re: [PATCH] leds: Add user LED driver for NIC78bx device

[Mika Westerberg]

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

Re: [PATCH] leds: Add user LED driver for NIC78bx device

[Rafael J. Wysocki]

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

Re: [PATCH] leds: Add user LED driver for NIC78bx device

[Hui Chun Ong]

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

Re: [PATCH] leds: Add user LED driver for NIC78bx device

[Julia Cartwright]

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

Re: [PATCH] leds: Add user LED driver for NIC78bx device

[Jacek Anaszewski]

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

Re: [PATCH] leds: Add user LED driver for NIC78bx device

[Hui Chun Ong]

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.