Re: [PATCH 11/20] Documentation: leds/ledtrig-transient: eliminate duplicated word
On 7/7/20 8:04 PM, Randy Dunlap wrote: Drop the doubled word "for". Signed-off-by: Randy Dunlap Cc: Jonathan Corbet Cc: linux-...@vger.kernel.org Cc: Jacek Anaszewski Cc: Pavel Machek Cc: Dan Murphy Cc: linux-l...@vger.kernel.org --- Documentation/leds/ledtrig-transient.rst |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) --- linux-next-20200701.orig/Documentation/leds/ledtrig-transient.rst +++ linux-next-20200701/Documentation/leds/ledtrig-transient.rst @@ -157,7 +157,7 @@ repeat the following step as needed:: echo 1 > activate - start timer = duration to run once echo none > trigger -This trigger is intended to be used for for the following example use cases: +This trigger is intended to be used for the following example use cases: - Control of vibrate (phones, tablets etc.) hardware by user space app. - Use of LED by user space app as activity indicator. Acked-by: Jacek Anaszewski -- Best regards, Jacek Anaszewski
Re: [PATCH] leds: powernv: add of_node_put()
Hi Yangtao,
Thank you for the patch.
On 11/21/2018 01:31 PM, Yangtao Li wrote:
> of_find_node_by_path() acquires a reference to the node returned by
> it and that reference needs to be dropped by its caller.bl_idle_init()
> doesn't do that, so fix it.
s/bl_idle_init/powernv_led_probe/
I suppose that you adopted the commit message from a fix
for drivers/cpuidle/cpuidle-big_little.c.
> Signed-off-by: Yangtao Li
> ---
> drivers/leds/leds-powernv.c | 12 +---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/leds/leds-powernv.c b/drivers/leds/leds-powernv.c
> index b1adbd70ce2e..0b1540029034 100644
> --- a/drivers/leds/leds-powernv.c
> +++ b/drivers/leds/leds-powernv.c
> @@ -285,6 +285,7 @@ static int powernv_led_probe(struct platform_device *pdev)
> struct device_node *led_node;
> struct powernv_led_common *powernv_led_common;
> struct device *dev = &pdev->dev;
> + int rc;
>
> led_node = of_find_node_by_path("/ibm,opal/leds");
> if (!led_node) {
> @@ -295,15 +296,20 @@ static int powernv_led_probe(struct platform_device
> *pdev)
>
> powernv_led_common = devm_kzalloc(dev, sizeof(*powernv_led_common),
> GFP_KERNEL);
> - if (!powernv_led_common)
> - return -ENOMEM;
> + if (!powernv_led_common){
missing space:
s/){/) {/
> + rc = -ENOMEM;
> + goto out;
> + }
>
> mutex_init(&powernv_led_common->lock);
> powernv_led_common->max_led_type = cpu_to_be64(OPAL_SLOT_LED_TYPE_MAX);
>
> platform_set_drvdata(pdev, powernv_led_common);
>
> - return powernv_led_classdev(pdev, led_node, powernv_led_common);
> + rc = powernv_led_classdev(pdev, led_node, powernv_led_common);
> +out:
> + of_node_put(led_node);
> + return rc;
> }
>
> /* Platform driver remove */
>
I've fixed those trivial problems and applied the patch
to the for-next branch of linux-leds.git.
--
Best regards,
Jacek Anaszewski
Re: [PATCH] leds-PowerNV: Delete an error message for a failed memory allocation in powernv_led_create()
Hi Markus,
Thanks for the patch.
On 08/27/2017 10:10 PM, SF Markus Elfring wrote:
> From: Markus Elfring
> Date: Sun, 27 Aug 2017 22:00:22 +0200
>
> Omit an extra message for a memory allocation failure in this function.
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring
> ---
> drivers/leds/leds-powernv.c | 6 +-
> 1 file changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/drivers/leds/leds-powernv.c b/drivers/leds/leds-powernv.c
> index b2a98c7b521b..b1adbd70ce2e 100644
> --- a/drivers/leds/leds-powernv.c
> +++ b/drivers/leds/leds-powernv.c
> @@ -224,12 +224,8 @@ static int powernv_led_create(struct device *dev,
> powernv_led->cdev.name = devm_kasprintf(dev, GFP_KERNEL, "%s:%s",
> powernv_led->loc_code,
> led_type_desc);
> - if (!powernv_led->cdev.name) {
> - dev_err(dev,
> - "%s: Memory allocation failed for classdev name\n",
> - __func__);
> + if (!powernv_led->cdev.name)
> return -ENOMEM;
> - }
>
> powernv_led->cdev.brightness_set_blocking = powernv_brightness_set;
> powernv_led->cdev.brightness_get = powernv_brightness_get;
>
Applied.
--
Best regards,
Jacek Anaszewski
Re: [PATCH] leds: Add no-op gpio_led_register_device when LED subsystem is disabled
On 06/21/2016 01:48 PM, Andrew F. Davis wrote:
On 06/21/2016 02:09 AM, Jacek Anaszewski wrote:
Hi Andrew,
This patch doesn't apply, please rebase onto recent LED tree.
On 06/21/2016 12:13 AM, Andrew F. Davis wrote:
Some systems use 'gpio_led_register_device' to make an in-memory copy of
their LED device table so the original can be removed as .init.rodata.
When the LED subsystem is not enabled source in the led directory is not
built and so this function may be undefined. Fix this here.
Signed-off-by: Andrew F. Davis
---
include/linux/leds.h | 8
1 file changed, 8 insertions(+)
diff --git a/include/linux/leds.h b/include/linux/leds.h
index d2b1306..a4a3da6 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -386,8 +386,16 @@ struct gpio_led_platform_data {
unsigned long *delay_off);
Currently there is some stuff here, and in fact it has been for
a long time.
Patch "[PATCH 12/12] leds: Only descend into leds directory when
CONFIG_NEW_LEDS is set" also doesn't apply.
What repository are you using?
v4.7-rc4, it may not apply due to the surrounding lines being changed in
the other patches which may not be applied to your tree. It is a single
line change per patch so hopefully the merge conflict resolutions will
be trivial.
A better solution could have been getting an ack from each maintainer
and having someone pull the whole series into one tree, but parts have
already been picked so it may be a little late for that.
OK, I resolved the issues and applied, thanks.
};
+#ifdef CONFIG_NEW_LEDS
struct platform_device *gpio_led_register_device(
int id, const struct gpio_led_platform_data *pdata);
+#else
+static inline struct platform_device *gpio_led_register_device(
+ int id, const struct gpio_led_platform_data *pdata)
+{
+ return 0;
+}
+#endif
enum cpu_led_event {
CPU_LED_IDLE_START, /* CPU enters idle */
--
Best regards,
Jacek Anaszewski
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] leds: Add no-op gpio_led_register_device when LED subsystem is disabled
Hi Andrew,
This patch doesn't apply, please rebase onto recent LED tree.
On 06/21/2016 12:13 AM, Andrew F. Davis wrote:
Some systems use 'gpio_led_register_device' to make an in-memory copy of
their LED device table so the original can be removed as .init.rodata.
When the LED subsystem is not enabled source in the led directory is not
built and so this function may be undefined. Fix this here.
Signed-off-by: Andrew F. Davis
---
include/linux/leds.h | 8
1 file changed, 8 insertions(+)
diff --git a/include/linux/leds.h b/include/linux/leds.h
index d2b1306..a4a3da6 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -386,8 +386,16 @@ struct gpio_led_platform_data {
unsigned long *delay_off);
Currently there is some stuff here, and in fact it has been for
a long time.
Patch "[PATCH 12/12] leds: Only descend into leds directory when
CONFIG_NEW_LEDS is set" also doesn't apply.
What repository are you using?
};
+#ifdef CONFIG_NEW_LEDS
struct platform_device *gpio_led_register_device(
int id, const struct gpio_led_platform_data *pdata);
+#else
+static inline struct platform_device *gpio_led_register_device(
+ int id, const struct gpio_led_platform_data *pdata)
+{
+ return 0;
+}
+#endif
enum cpu_led_event {
CPU_LED_IDLE_START, /* CPU enters idle */
--
Best regards,
Jacek Anaszewski
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 12/12] leds: Only descend into leds directory when CONFIG_NEW_LEDS is set
On 06/18/2016 12:46 AM, Andrew F. Davis wrote: On 06/15/2016 01:48 AM, Jacek Anaszewski wrote: Hi Andrew, Thanks for the patch. Please address the issue [1] raised by test bot and resubmit. Thanks, Jacek Anaszewski [1] https://lkml.org/lkml/2016/6/13/1091 It looks like some systems use 'gpio_led_register_device' to make an in-memory copy of their LED device table so the original can be removed as .init.rodata. This doesn't necessarily depend on the LED subsystem but it kind of seems useless when the rest of the subsystem is disabled. One solution could be to use a dummy 'gpio_led_register_device' when the subsystem is not enabled. It sounds good. Please add a no-op version of gpio_led_register_device() to include/leds.h, in a separate patch. Thanks, Jacek Anaszewski Another is just to remove the five or so uses of 'gpio_led_register_device' and have those systems register LED device tables like other systems do. If nether of these are acceptable then this patch can be dropped from this series for now. Thanks, Andrew On 06/13/2016 10:02 PM, Andrew F. Davis wrote: When CONFIG_NEW_LEDS is not set make will still descend into the leds directory but nothing will be built. This produces unneeded build artifacts and messages in addition to slowing the build. Fix this here. Signed-off-by: Andrew F. Davis --- drivers/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/Makefile b/drivers/Makefile index 567e32c..fa514d5 100644 --- a/drivers/Makefile +++ b/drivers/Makefile @@ -127,7 +127,7 @@ obj-$(CONFIG_CPU_FREQ)+= cpufreq/ obj-$(CONFIG_CPU_IDLE)+= cpuidle/ obj-$(CONFIG_MMC)+= mmc/ obj-$(CONFIG_MEMSTICK)+= memstick/ -obj-y+= leds/ +obj-$(CONFIG_NEW_LEDS)+= leds/ obj-$(CONFIG_INFINIBAND)+= infiniband/ obj-$(CONFIG_SGI_SN)+= sn/ obj-y+= firmware/ ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH 12/12] leds: Only descend into leds directory when CONFIG_NEW_LEDS is set
Hi Andrew, Thanks for the patch. Please address the issue [1] raised by test bot and resubmit. Thanks, Jacek Anaszewski [1] https://lkml.org/lkml/2016/6/13/1091 On 06/13/2016 10:02 PM, Andrew F. Davis wrote: When CONFIG_NEW_LEDS is not set make will still descend into the leds directory but nothing will be built. This produces unneeded build artifacts and messages in addition to slowing the build. Fix this here. Signed-off-by: Andrew F. Davis --- drivers/Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/Makefile b/drivers/Makefile index 567e32c..fa514d5 100644 --- a/drivers/Makefile +++ b/drivers/Makefile @@ -127,7 +127,7 @@ obj-$(CONFIG_CPU_FREQ) += cpufreq/ obj-$(CONFIG_CPU_IDLE)+= cpuidle/ obj-$(CONFIG_MMC) += mmc/ obj-$(CONFIG_MEMSTICK)+= memstick/ -obj-y += leds/ +obj-$(CONFIG_NEW_LEDS) += leds/ obj-$(CONFIG_INFINIBAND) += infiniband/ obj-$(CONFIG_SGI_SN) += sn/ obj-y += firmware/ -- Best regards, Jacek Anaszewski ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v4 3/7] powerpc: use the new LED disk activity trigger
Hi all,
For consistency reasons this patch should be merged through LED tree,
but I need an ack from relevant maintainer. Benjamin, Michael, Paul?
Thanks,
Jacek Anaszewski
On 06/10/2016 07:59 AM, Stephan Linz wrote:
- dts: rename 'ide-disk' to 'disk-activity'
- defconfig: rename 'ADB_PMU_LED_IDE' to 'ADB_PMU_LED_DISK'
Cc: Joseph Jezak
Cc: Jörg Sommer
Signed-off-by: Stephan Linz
---
arch/powerpc/boot/dts/mpc8315erdb.dts | 2 +-
arch/powerpc/boot/dts/mpc8377_rdb.dts | 2 +-
arch/powerpc/boot/dts/mpc8378_rdb.dts | 2 +-
arch/powerpc/boot/dts/mpc8379_rdb.dts | 2 +-
arch/powerpc/configs/pmac32_defconfig | 2 +-
arch/powerpc/configs/ppc6xx_defconfig | 2 +-
drivers/macintosh/Kconfig | 13 ++---
drivers/macintosh/via-pmu-led.c | 4 ++--
8 files changed, 14 insertions(+), 15 deletions(-)
diff --git a/arch/powerpc/boot/dts/mpc8315erdb.dts
b/arch/powerpc/boot/dts/mpc8315erdb.dts
index 4354684..ca5139e 100644
--- a/arch/powerpc/boot/dts/mpc8315erdb.dts
+++ b/arch/powerpc/boot/dts/mpc8315erdb.dts
@@ -472,7 +472,7 @@
hdd {
gpios = <&mcu_pio 1 0>;
- linux,default-trigger = "ide-disk";
+ linux,default-trigger = "disk-activity";
};
};
};
diff --git a/arch/powerpc/boot/dts/mpc8377_rdb.dts
b/arch/powerpc/boot/dts/mpc8377_rdb.dts
index 2b4b653..e326139 100644
--- a/arch/powerpc/boot/dts/mpc8377_rdb.dts
+++ b/arch/powerpc/boot/dts/mpc8377_rdb.dts
@@ -496,7 +496,7 @@
hdd {
gpios = <&mcu_pio 1 0>;
- linux,default-trigger = "ide-disk";
+ linux,default-trigger = "disk-activity";
};
};
};
diff --git a/arch/powerpc/boot/dts/mpc8378_rdb.dts
b/arch/powerpc/boot/dts/mpc8378_rdb.dts
index 74b6a53..71842fc 100644
--- a/arch/powerpc/boot/dts/mpc8378_rdb.dts
+++ b/arch/powerpc/boot/dts/mpc8378_rdb.dts
@@ -480,7 +480,7 @@
hdd {
gpios = <&mcu_pio 1 0>;
- linux,default-trigger = "ide-disk";
+ linux,default-trigger = "disk-activity";
};
};
};
diff --git a/arch/powerpc/boot/dts/mpc8379_rdb.dts
b/arch/powerpc/boot/dts/mpc8379_rdb.dts
index 3b5cbac..e442a29 100644
--- a/arch/powerpc/boot/dts/mpc8379_rdb.dts
+++ b/arch/powerpc/boot/dts/mpc8379_rdb.dts
@@ -446,7 +446,7 @@
hdd {
gpios = <&mcu_pio 1 0>;
- linux,default-trigger = "ide-disk";
+ linux,default-trigger = "disk-activity";
};
};
};
diff --git a/arch/powerpc/configs/pmac32_defconfig
b/arch/powerpc/configs/pmac32_defconfig
index ea8705f..3f6c9a6 100644
--- a/arch/powerpc/configs/pmac32_defconfig
+++ b/arch/powerpc/configs/pmac32_defconfig
@@ -158,7 +158,7 @@ CONFIG_ADB=y
CONFIG_ADB_CUDA=y
CONFIG_ADB_PMU=y
CONFIG_ADB_PMU_LED=y
-CONFIG_ADB_PMU_LED_IDE=y
+CONFIG_ADB_PMU_LED_DISK=y
CONFIG_PMAC_APM_EMU=m
CONFIG_PMAC_MEDIABAY=y
CONFIG_PMAC_BACKLIGHT=y
diff --git a/arch/powerpc/configs/ppc6xx_defconfig
b/arch/powerpc/configs/ppc6xx_defconfig
index 99ccbeba..1dde0be 100644
--- a/arch/powerpc/configs/ppc6xx_defconfig
+++ b/arch/powerpc/configs/ppc6xx_defconfig
@@ -442,7 +442,7 @@ CONFIG_ADB=y
CONFIG_ADB_CUDA=y
CONFIG_ADB_PMU=y
CONFIG_ADB_PMU_LED=y
-CONFIG_ADB_PMU_LED_IDE=y
+CONFIG_ADB_PMU_LED_DISK=y
CONFIG_PMAC_APM_EMU=y
CONFIG_PMAC_MEDIABAY=y
CONFIG_PMAC_BACKLIGHT=y
diff --git a/drivers/macintosh/Kconfig b/drivers/macintosh/Kconfig
index 3e8b29e..d28690f 100644
--- a/drivers/macintosh/Kconfig
+++ b/drivers/macintosh/Kconfig
@@ -96,19 +96,18 @@ config ADB_PMU_LED
Support the front LED on Power/iBooks as a generic LED that can
be triggered by any of the supported triggers. To get the
behaviour of the old CONFIG_BLK_DEV_IDE_PMAC_BLINK, select this
- and the ide-disk LED trigger and configure appropriately through
- sysfs.
+ and the disk LED trigger and configure appropriately through sysfs.
-config ADB_PMU_LED_IDE
- bool "Use front LED as IDE LED by default"
+config ADB_PMU_LED_DISK
+ bool "Use front LED as DISK LED by default"
depends on ADB_PMU_LED
depends on LEDS_CLASS
depends on IDE_GD_ATA
select LEDS_TRIGGERS
- select LEDS_TRIGGER_IDE_DISK
+ select LEDS_TRIGGER_DISK
help
- This option makes the front LED default to the IDE trigger
- so that it blinks on IDE activity.
+ This option makes the front LED default to the disk trigger
+ so that it blinks on disk activity.
config PMAC_SMU
bool "Support for SMU based PowerMacs"
diff --gi
[PATCH] leds: powernv: Implement brightness_set_blocking op
Since brightness setting can sleep for this driver, implement
brightness_set_blocking op, instead of brightness_set.
It makes this driver compatible with LED triggers.
Signed-off-by: Jacek Anaszewski
Cc: Vasant Hegde
---
drivers/leds/leds-powernv.c | 16 ++--
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/leds/leds-powernv.c b/drivers/leds/leds-powernv.c
index 1e75e1f..dd76f34 100644
--- a/drivers/leds/leds-powernv.c
+++ b/drivers/leds/leds-powernv.c
@@ -77,7 +77,7 @@ static int powernv_get_led_type(const char *led_type_desc)
* This function is called from work queue task context when ever it gets
* scheduled. This function can sleep at opal_async_wait_response call.
*/
-static void powernv_led_set(struct powernv_led_data *powernv_led,
+static int powernv_led_set(struct powernv_led_data *powernv_led,
enum led_brightness value)
{
int rc, token;
@@ -99,7 +99,7 @@ static void powernv_led_set(struct powernv_led_data
*powernv_led,
if (token != -ERESTARTSYS)
dev_err(dev, "%s: Couldn't get OPAL async token\n",
__func__);
- return;
+ return token;
}
rc = opal_leds_set_ind(token, powernv_led->loc_code,
@@ -125,6 +125,7 @@ static void powernv_led_set(struct powernv_led_data
*powernv_led,
out_token:
opal_async_release_token(token);
+ return rc;
}
/*
@@ -173,20 +174,23 @@ static enum led_brightness powernv_led_get(struct
powernv_led_data *powernv_led)
* LED classdev 'brightness_get' function. This schedules work
* to update LED state.
*/
-static void powernv_brightness_set(struct led_classdev *led_cdev,
+static int powernv_brightness_set(struct led_classdev *led_cdev,
enum led_brightness value)
{
struct powernv_led_data *powernv_led =
container_of(led_cdev, struct powernv_led_data, cdev);
struct powernv_led_common *powernv_led_common = powernv_led->common;
+ int rc;
/* Do not modify LED in unload path */
if (powernv_led_common->led_disabled)
- return;
+ return 0;
mutex_lock(&powernv_led_common->lock);
- powernv_led_set(powernv_led, value);
+ rc = powernv_led_set(powernv_led, value);
mutex_unlock(&powernv_led_common->lock);
+
+ return rc;
}
/* LED classdev 'brightness_get' function */
@@ -227,7 +231,7 @@ static int powernv_led_create(struct device *dev,
return -ENOMEM;
}
- powernv_led->cdev.brightness_set = powernv_brightness_set;
+ powernv_led->cdev.brightness_set_blocking = powernv_brightness_set;
powernv_led->cdev.brightness_get = powernv_brightness_get;
powernv_led->cdev.brightness = LED_OFF;
powernv_led->cdev.max_brightness = LED_FULL;
--
1.7.9.5
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH] leds: powernv: Implement brightness_set_blocking op
Since brightness setting can sleep for this driver, implement
brightness_set_blocking op, instead of brightness_set.
It makes this driver compatible with LED triggers.
Signed-off-by: Jacek Anaszewski
Cc: Vasant Hegde
---
drivers/leds/leds-powernv.c | 16 ++--
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/leds/leds-powernv.c b/drivers/leds/leds-powernv.c
index 1e75e1f..dd76f34 100644
--- a/drivers/leds/leds-powernv.c
+++ b/drivers/leds/leds-powernv.c
@@ -77,7 +77,7 @@ static int powernv_get_led_type(const char *led_type_desc)
* This function is called from work queue task context when ever it gets
* scheduled. This function can sleep at opal_async_wait_response call.
*/
-static void powernv_led_set(struct powernv_led_data *powernv_led,
+static int powernv_led_set(struct powernv_led_data *powernv_led,
enum led_brightness value)
{
int rc, token;
@@ -99,7 +99,7 @@ static void powernv_led_set(struct powernv_led_data
*powernv_led,
if (token != -ERESTARTSYS)
dev_err(dev, "%s: Couldn't get OPAL async token\n",
__func__);
- return;
+ return token;
}
rc = opal_leds_set_ind(token, powernv_led->loc_code,
@@ -125,6 +125,7 @@ static void powernv_led_set(struct powernv_led_data
*powernv_led,
out_token:
opal_async_release_token(token);
+ return rc;
}
/*
@@ -173,20 +174,23 @@ static enum led_brightness powernv_led_get(struct
powernv_led_data *powernv_led)
* LED classdev 'brightness_get' function. This schedules work
* to update LED state.
*/
-static void powernv_brightness_set(struct led_classdev *led_cdev,
+static int powernv_brightness_set(struct led_classdev *led_cdev,
enum led_brightness value)
{
struct powernv_led_data *powernv_led =
container_of(led_cdev, struct powernv_led_data, cdev);
struct powernv_led_common *powernv_led_common = powernv_led->common;
+ int rc;
/* Do not modify LED in unload path */
if (powernv_led_common->led_disabled)
- return;
+ return 0;
mutex_lock(&powernv_led_common->lock);
- powernv_led_set(powernv_led, value);
+ rc = powernv_led_set(powernv_led, value);
mutex_unlock(&powernv_led_common->lock);
+
+ return rc;
}
/* LED classdev 'brightness_get' function */
@@ -227,7 +231,7 @@ static int powernv_led_create(struct device *dev,
return -ENOMEM;
}
- powernv_led->cdev.brightness_set = powernv_brightness_set;
+ powernv_led->cdev.brightness_set_blocking = powernv_brightness_set;
powernv_led->cdev.brightness_get = powernv_brightness_get;
powernv_led->cdev.brightness = LED_OFF;
powernv_led->cdev.max_brightness = LED_FULL;
--
1.7.9.5
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v9 3/3] leds/powernv: Add driver for PowerNV platform
Hi Vasant,
I've found two superfluous lines. Please find my remarks below.
On 08/19/2015 12:41 PM, Vasant Hegde wrote:
This patch implements LED driver for PowerNV platform using the existing
generic LED class framework.
PowerNV platform has below type of LEDs:
- System attention
Indicates there is a problem with the system that needs attention.
- Identify
Helps the user locate/identify a particular FRU or resource in the
system.
- Fault
Indicates there is a problem with the FRU or resource at the
location with which the indicator is associated.
We register classdev structures for all individual LEDs detected on the
system through LED specific device tree nodes. Device tree nodes specify
what all kind of LEDs present on the same location code. It registers
LED classdev structure for each of them.
All the system LEDs can be found in the same regular path /sys/class/leds/.
We don't use LED colors. We use LED node and led-types property to form
LED classdev. Our LEDs have names in this format.
:
Any positive brightness value would turn on the LED and a zero value would
turn off the LED. The driver will return LED_FULL (255) for any turned on
LED and LED_OFF (0) for any turned off LED.
The platform level implementation of LED get and set state has been
achieved through OPAL calls. These calls are made available for the
driver by exporting from architecture specific codes.
Signed-off-by: Vasant Hegde
Signed-off-by: Anshuman Khandual
Acked-by: Stewart Smith
Tested-by: Stewart Smith
Acked-by: Jacek Anaszewski
---
.../devicetree/bindings/leds/leds-powernv.txt | 26 ++
drivers/leds/Kconfig | 11 +
drivers/leds/Makefile | 1 +
drivers/leds/leds-powernv.c| 347 +
4 files changed, 385 insertions(+)
create mode 100644 Documentation/devicetree/bindings/leds/leds-powernv.txt
create mode 100644 drivers/leds/leds-powernv.c
diff --git a/Documentation/devicetree/bindings/leds/leds-powernv.txt
b/Documentation/devicetree/bindings/leds/leds-powernv.txt
new file mode 100644
index 000..6665569
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-powernv.txt
@@ -0,0 +1,26 @@
+Device Tree binding for LEDs on IBM Power Systems
+-
+
+Required properties:
+- compatible : Should be "ibm,opal-v3-led".
+- led-mode : Should be "lightpath" or "guidinglight".
+
+Each location code of FRU/Enclosure must be expressed in the
+form of a sub-node.
+
+Required properties for the sub nodes:
+- led-types : Supported LED types (attention/identify/fault) provided
+ in the form of string array.
+
+Example:
+
+leds {
+ compatible = "ibm,opal-v3-led";
+ led-mode = "lightpath";
+
+ U78C9.001.RST0027-P1-C1 {
+ led-types = "identify", "fault";
+ };
+ ...
+ ...
+};
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 9ad35f7..f218cc3a 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -560,6 +560,17 @@ config LEDS_BLINKM
This option enables support for the BlinkM RGB LED connected
through I2C. Say Y to enable support for the BlinkM LED.
+config LEDS_POWERNV
+ tristate "LED support for PowerNV Platform"
+ depends on LEDS_CLASS
+ depends on PPC_POWERNV
+ depends on OF
+ help
+ This option enables support for the system LEDs present on
+ PowerNV platforms. Say 'y' to enable this support in kernel.
+ To compile this driver as a module, choose 'm' here: the module
+ will be called leds-powernv.
+
config LEDS_SYSCON
bool "LED support for LEDs on system controllers"
depends on LEDS_CLASS=y
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 8d6a24a..6a943d1 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -65,6 +65,7 @@ obj-$(CONFIG_LEDS_VERSATILE) += leds-versatile.o
obj-$(CONFIG_LEDS_MENF21BMC) += leds-menf21bmc.o
obj-$(CONFIG_LEDS_PM8941_WLED)+= leds-pm8941-wled.o
obj-$(CONFIG_LEDS_KTD2692)+= leds-ktd2692.o
+obj-$(CONFIG_LEDS_POWERNV) += leds-powernv.o
# LED SPI Drivers
obj-$(CONFIG_LEDS_DAC124S085) += leds-dac124s085.o
diff --git a/drivers/leds/leds-powernv.c b/drivers/leds/leds-powernv.c
new file mode 100644
index 000..ac3fdf5
--- /dev/null
+++ b/drivers/leds/leds-powernv.c
@@ -0,0 +1,347 @@
+/*
+ * PowerNV LED Driver
+ *
+ * Copyright IBM Corp. 2015
+ *
+ * Author: Vasant Hegde
+ * Author: Anshuman Khandual
+ *
+ * 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 Founda
Re: [PATCH v8 3/3] leds/powernv: Add driver for PowerNV platform
Hi Vasant, On 07/28/2015 07:40 PM, Jacek Anaszewski wrote: Vasant, On 28.07.2015 15:40, Vasant Hegde wrote: On 07/27/2015 03:20 PM, Jacek Anaszewski wrote: Hi Vasant, On 27.07.2015 05:41, Vasant Hegde wrote: On 07/27/2015 03:11 AM, Jacek Anaszewski wrote: Hi Vasant, Hi Jacek, Two trivial details left. Please find them below. Thanks for the review/Ack. I'll fix below issues and resend patchset. I will ask Benh/Michael to take this patchset. But this patchset is depending on your core changes. Can you confirm that you are pushing that patchset in next merge window? Jacek, Without my core changes your driver won't work with led triggers, but AFAIR this use case is not relevant for your LEDs? Eventually, we could produce a patch set adding support for LED triggers if it will be clear that LED core changes will not be merged in the upcoming merge window. IIUC current LED code doesn't allow me to sleep (without driver specific workqueue). And powernv_led_set() call will sleep. Hence I think it won't work. I did a quick test without your patch. It doesn't seems to be working. Does brightness setting work properly without work queue? -- Best Regards, Jacek Anaszewski ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v8 3/3] leds/powernv: Add driver for PowerNV platform
Vasant, On 28.07.2015 15:40, Vasant Hegde wrote: On 07/27/2015 03:20 PM, Jacek Anaszewski wrote: Hi Vasant, On 27.07.2015 05:41, Vasant Hegde wrote: On 07/27/2015 03:11 AM, Jacek Anaszewski wrote: Hi Vasant, Hi Jacek, Two trivial details left. Please find them below. Thanks for the review/Ack. I'll fix below issues and resend patchset. I will ask Benh/Michael to take this patchset. But this patchset is depending on your core changes. Can you confirm that you are pushing that patchset in next merge window? Jacek, Without my core changes your driver won't work with led triggers, but AFAIR this use case is not relevant for your LEDs? Eventually, we could produce a patch set adding support for LED triggers if it will be clear that LED core changes will not be merged in the upcoming merge window. IIUC current LED code doesn't allow me to sleep (without driver specific workqueue). And powernv_led_set() call will sleep. Hence I think it won't work. I did a quick test without your patch. It doesn't seems to be working. Strange, it should work when led_set_brightness is called from non atomic context, e.g. from sysfs interface. Could you share your test scenario and describe the symptoms? Alternatively we can revert the changes (add driver specific workqueue now) and later when your changes goes to upstream, I can fix my code. You can add this as an incremental patch which would be easy to revert when LED core changes are merged. -- Best Regards, Jacek Anaszewski ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v8 3/3] leds/powernv: Add driver for PowerNV platform
On 28.07.2015 12:14, Benjamin Herrenschmidt wrote:
On Tue, 2015-07-28 at 08:38 +0200, Jacek Anaszewski wrote:
+/* Register the classdev */
+rc = devm_led_classdev_register(dev, &powernv_led->cdev);
+if (rc) {
+dev_err(dev, "%s: Classdev registration failed for %s\n",
+__func__, powernv_led->cdev.name);
+}
Braces are not needed here,
Putting my old maintainer hat on for a minute ... this is serious nit
picking :-)
Arguably, the function being multi-line, the braces do make the code
more readable. This is a matter of taste which in such as case
which isn't a blatant violation of an important rule should be left to
the choice of the code implementor/maintainer.
I will not insist as checkpatch.pl also doesn't complain
about that.
Vasant, please ignore my remark then.
--
Best Regards,
Jacek Anaszewski
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v8 3/3] leds/powernv: Add driver for PowerNV platform
Vasant,
I've found one more formatting issue below.
On 27.07.2015 05:41, Vasant Hegde wrote:
[...]
+
+/*
+ * This function registers classdev structure for any given type of LED on
+ * a given child LED device node.
+ */
+static int powernv_led_create(struct device *dev,
+ struct powernv_led_data *powernv_led,
+ const char *led_type_desc)
+{
+int rc;
+
+/* Make sure LED type is supported */
+powernv_led->led_type = powernv_get_led_type(led_type_desc);
+if (powernv_led->led_type == -1) {
+dev_warn(dev, "%s: No support for led type : %s\n",
+ __func__, led_type_desc);
+return -EINVAL;
+}
+
+/* Create the name for classdev */
+powernv_led->cdev.name = devm_kasprintf(dev, GFP_KERNEL, "%s:%s",
+powernv_led->loc_code,
+led_type_desc);
+if (!powernv_led->cdev.name) {
+dev_err(dev,
+"%s: Memory allocation failed for classdev name\n",
+__func__);
+return -ENOMEM;
+}
+
+powernv_led->cdev.brightness_set = powernv_brightness_set;
+powernv_led->cdev.brightness_get = powernv_brightness_get;
+powernv_led->cdev.brightness = LED_OFF;
+powernv_led->cdev.max_brightness = LED_FULL;
+
+/* Register the classdev */
+rc = devm_led_classdev_register(dev, &powernv_led->cdev);
+if (rc) {
+dev_err(dev, "%s: Classdev registration failed for %s\n",
+__func__, powernv_led->cdev.name);
+ }
Braces are not needed here,
+
+return rc;
+}
--
Best Regards,
Jacek Anaszewski
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v8 3/3] leds/powernv: Add driver for PowerNV platform
Hi Vasant,
On 27.07.2015 05:41, Vasant Hegde wrote:
On 07/27/2015 03:11 AM, Jacek Anaszewski wrote:
Hi Vasant,
Hi Jacek,
Two trivial details left. Please find them below.
Thanks for the review/Ack. I'll fix below issues and resend patchset.
I will ask Benh/Michael to take this patchset. But this patchset is depending
on your core changes. Can you confirm that you are pushing that patchset in next
merge window?
Without my core changes your driver won't work with led triggers, but
AFAIR this use case is not relevant for your LEDs? Eventually, we could
produce a patch set adding support for LED triggers if it will be clear
that LED core changes will not be merged in the upcoming merge window.
-Vasant
Since for two next weeks I will be unable even to compile-test
this patch set I propose to merge it via powerpc tree.
Having both mentioned issues addressed, for this patch:
Acked-by: Jacek Anaszewski
On 25.07.2015 07:21, Vasant Hegde wrote:
This patch implements LED driver for PowerNV platform using the existing
generic LED class framework.
PowerNV platform has below type of LEDs:
- System attention
Indicates there is a problem with the system that needs attention.
- Identify
Helps the user locate/identify a particular FRU or resource in the
system.
- Fault
Indicates there is a problem with the FRU or resource at the
location with which the indicator is associated.
We register classdev structures for all individual LEDs detected on the
system through LED specific device tree nodes. Device tree nodes specify
what all kind of LEDs present on the same location code. It registers
LED classdev structure for each of them.
All the system LEDs can be found in the same regular path /sys/class/leds/.
We don't use LED colors. We use LED node and led-types property to form
LED classdev. Our LEDs have names in this format.
:
Any positive brightness value would turn on the LED and a zero value would
turn off the LED. The driver will return LED_FULL (255) for any turned on
LED and LED_OFF (0) for any turned off LED.
As per the LED class framework, the 'brightness_set' function should not
sleep. Hence these functions have been implemented through global work
queue tasks which might sleep on OPAL async call completion.
This is no longer true.
The platform level implementation of LED get and set state has been
achieved through OPAL calls. These calls are made available for the
driver by exporting from architecture specific codes.
Signed-off-by: Vasant Hegde
Signed-off-by: Anshuman Khandual
Acked-by: Stewart Smith
Tested-by: Stewart Smith
---
.../devicetree/bindings/leds/leds-powernv.txt | 26 ++
drivers/leds/Kconfig | 11 +
drivers/leds/Makefile | 1 +
drivers/leds/leds-powernv.c| 350
+
4 files changed, 388 insertions(+)
create mode 100644 Documentation/devicetree/bindings/leds/leds-powernv.txt
create mode 100644 drivers/leds/leds-powernv.c
diff --git a/Documentation/devicetree/bindings/leds/leds-powernv.txt
b/Documentation/devicetree/bindings/leds/leds-powernv.txt
new file mode 100644
index 000..6665569
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-powernv.txt
@@ -0,0 +1,26 @@
+Device Tree binding for LEDs on IBM Power Systems
+-
+
+Required properties:
+- compatible : Should be "ibm,opal-v3-led".
+- led-mode : Should be "lightpath" or "guidinglight".
+
+Each location code of FRU/Enclosure must be expressed in the
+form of a sub-node.
+
+Required properties for the sub nodes:
+- led-types : Supported LED types (attention/identify/fault) provided
+ in the form of string array.
+
+Example:
+
+leds {
+compatible = "ibm,opal-v3-led";
+led-mode = "lightpath";
+
+U78C9.001.RST0027-P1-C1 {
+led-types = "identify", "fault";
+};
+...
+...
+};
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 9ad35f7..f218cc3a 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -560,6 +560,17 @@ config LEDS_BLINKM
This option enables support for the BlinkM RGB LED connected
through I2C. Say Y to enable support for the BlinkM LED.
+config LEDS_POWERNV
+tristate "LED support for PowerNV Platform"
+depends on LEDS_CLASS
+depends on PPC_POWERNV
+depends on OF
+help
+ This option enables support for the system LEDs present on
+ PowerNV platforms. Say 'y' to enable this support in kernel.
+ To compile this driver as a module, choose 'm' here: the module
+ will be called leds-powernv.
+
config LEDS_SYSCON
bool "LED support for LEDs on system controllers"
depends on LEDS_CLASS=y
Re: [PATCH v8 3/3] leds/powernv: Add driver for PowerNV platform
Hi Vasant,
Two trivial details left. Please find them below.
Since for two next weeks I will be unable even to compile-test
this patch set I propose to merge it via powerpc tree.
Having both mentioned issues addressed, for this patch:
Acked-by: Jacek Anaszewski
On 25.07.2015 07:21, Vasant Hegde wrote:
This patch implements LED driver for PowerNV platform using the existing
generic LED class framework.
PowerNV platform has below type of LEDs:
- System attention
Indicates there is a problem with the system that needs attention.
- Identify
Helps the user locate/identify a particular FRU or resource in the
system.
- Fault
Indicates there is a problem with the FRU or resource at the
location with which the indicator is associated.
We register classdev structures for all individual LEDs detected on the
system through LED specific device tree nodes. Device tree nodes specify
what all kind of LEDs present on the same location code. It registers
LED classdev structure for each of them.
All the system LEDs can be found in the same regular path /sys/class/leds/.
We don't use LED colors. We use LED node and led-types property to form
LED classdev. Our LEDs have names in this format.
:
Any positive brightness value would turn on the LED and a zero value would
turn off the LED. The driver will return LED_FULL (255) for any turned on
LED and LED_OFF (0) for any turned off LED.
As per the LED class framework, the 'brightness_set' function should not
sleep. Hence these functions have been implemented through global work
queue tasks which might sleep on OPAL async call completion.
This is no longer true.
The platform level implementation of LED get and set state has been
achieved through OPAL calls. These calls are made available for the
driver by exporting from architecture specific codes.
Signed-off-by: Vasant Hegde
Signed-off-by: Anshuman Khandual
Acked-by: Stewart Smith
Tested-by: Stewart Smith
---
.../devicetree/bindings/leds/leds-powernv.txt | 26 ++
drivers/leds/Kconfig | 11 +
drivers/leds/Makefile | 1 +
drivers/leds/leds-powernv.c| 350 +
4 files changed, 388 insertions(+)
create mode 100644 Documentation/devicetree/bindings/leds/leds-powernv.txt
create mode 100644 drivers/leds/leds-powernv.c
diff --git a/Documentation/devicetree/bindings/leds/leds-powernv.txt
b/Documentation/devicetree/bindings/leds/leds-powernv.txt
new file mode 100644
index 000..6665569
--- /dev/null
+++ b/Documentation/devicetree/bindings/leds/leds-powernv.txt
@@ -0,0 +1,26 @@
+Device Tree binding for LEDs on IBM Power Systems
+-
+
+Required properties:
+- compatible : Should be "ibm,opal-v3-led".
+- led-mode : Should be "lightpath" or "guidinglight".
+
+Each location code of FRU/Enclosure must be expressed in the
+form of a sub-node.
+
+Required properties for the sub nodes:
+- led-types : Supported LED types (attention/identify/fault) provided
+ in the form of string array.
+
+Example:
+
+leds {
+ compatible = "ibm,opal-v3-led";
+ led-mode = "lightpath";
+
+ U78C9.001.RST0027-P1-C1 {
+ led-types = "identify", "fault";
+ };
+ ...
+ ...
+};
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 9ad35f7..f218cc3a 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -560,6 +560,17 @@ config LEDS_BLINKM
This option enables support for the BlinkM RGB LED connected
through I2C. Say Y to enable support for the BlinkM LED.
+config LEDS_POWERNV
+ tristate "LED support for PowerNV Platform"
+ depends on LEDS_CLASS
+ depends on PPC_POWERNV
+ depends on OF
+ help
+ This option enables support for the system LEDs present on
+ PowerNV platforms. Say 'y' to enable this support in kernel.
+ To compile this driver as a module, choose 'm' here: the module
+ will be called leds-powernv.
+
config LEDS_SYSCON
bool "LED support for LEDs on system controllers"
depends on LEDS_CLASS=y
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 8d6a24a..6a943d1 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -65,6 +65,7 @@ obj-$(CONFIG_LEDS_VERSATILE) += leds-versatile.o
obj-$(CONFIG_LEDS_MENF21BMC) += leds-menf21bmc.o
obj-$(CONFIG_LEDS_PM8941_WLED)+= leds-pm8941-wled.o
obj-$(CONFIG_LEDS_KTD2692)+= leds-ktd2692.o
+obj-$(CONFIG_LEDS_POWERNV) += leds-powernv.o
# LED SPI Drivers
obj-$(CONFIG_LEDS_DAC124S085) += leds-dac124s085.o
diff --git a/drivers/leds/leds-powernv.c b/drivers/leds/leds-powernv.c
new file mo
Re: [PATCH v7 3/3] leds/powernv: Add driver for PowerNV platform
Vasant,
On 23.07.2015 10:08, Vasant Hegde wrote:
On 07/23/2015 01:25 PM, Jacek Anaszewski wrote:
Hi Vasant,
Jacek,
.../...
+/* PowerNV LED data */
+struct powernv_led_data {
+struct led_classdevcdev;
+char*loc_code;/* LED location code */
+intled_type;/* OPAL_SLOT_LED_TYPE_* */
+enum led_brightnessvalue;/* Brightness value */
You don't need 'value' as brightness value is already stored in
cdev.brightness.
Agree. I'll remove.
+/*
+ * LED classdev 'brightness_get' function. This schedules work
+ * to update LED state.
+ */
+static void powernv_brightness_set(struct led_classdev *led_cdev,
+ enum led_brightness value)
+{
+struct powernv_led_data *powernv_led =
+container_of(led_cdev, struct powernv_led_data, cdev);
+
+/* Do not modify LED in unload path */
+if (led_disabled)
+return;
+
+/* Prepare the request */
+powernv_led->value = value;
+
+ return powernv_led_set(powernv_led);
Isn't mutex_lock/unlock missing here?
Yes. I realized this after sending out the patch. I will fix this.
.../...
+
+max_led_type = devm_kzalloc(dev, sizeof(*max_led_type), GFP_KERNEL);
+if (!max_led_type)
+return -ENOMEM;
+
+mutex_init(lock);
+*max_led_type = cpu_to_be64(OPAL_SLOT_LED_TYPE_MAX);
+
+platform_set_drvdata(pdev, lock);
Setting only lock as drvdata looks odd to me and I haven't noticed
anyone doing that.
I'd prefer to put lock, led_disabled and max_led_type in a common
struct and set it as drvdata. I know that I accepted this design, but
I didn't take into account these details.
Yeah. Even I looked into existing code and I don't see anyone using like this.
Since it's void * and we just need lock, I added like this.
If I break this into two structure, then I've to use platform_get_drvdata() call
in multiple functions like powernv_brightness_set() to get max_let_type etc. Is
that fine?
You could add wrappers for grouping instructions required to
retrieve given properties of the common struct.
Alternatively you could add a pointer to the common struct
in the struct powernv_led_data. You can play with these approaches
and choose the one which looks better.
--
Best Regards,
Jacek Anaszewski
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v7 3/3] leds/powernv: Add driver for PowerNV platform
loc(dev, sizeof(*max_led_type), GFP_KERNEL);
+ if (!max_led_type)
+ return -ENOMEM;
+
+ mutex_init(lock);
+ *max_led_type = cpu_to_be64(OPAL_SLOT_LED_TYPE_MAX);
+
+ platform_set_drvdata(pdev, lock);
Setting only lock as drvdata looks odd to me and I haven't noticed
anyone doing that.
I'd prefer to put lock, led_disabled and max_led_type in a common
struct and set it as drvdata. I know that I accepted this design, but
I didn't take into account these details.
+ return powernv_led_classdev(pdev, led_node, max_led_type, lock);
+}
+
+/* Platform driver remove */
+static int powernv_led_remove(struct platform_device *pdev)
+{
+ struct mutex *lock = platform_get_drvdata(pdev);
+
+ /* Disable LED operation */
+ led_disabled = true;
+
+ mutex_destroy(lock);
+
+ dev_info(&pdev->dev, "PowerNV led module unregistered\n");
+ return 0;
+}
+
+/* Platform driver property match */
+static const struct of_device_id powernv_led_match[] = {
+ {
+ .compatible = "ibm,opal-v3-led",
+ },
+ {},
+};
+MODULE_DEVICE_TABLE(of, powernv_led_match);
+
+static struct platform_driver powernv_led_driver = {
+ .probe = powernv_led_probe,
+ .remove = powernv_led_remove,
+ .driver = {
+ .name = "powernv-led-driver",
+ .owner = THIS_MODULE,
+ .of_match_table = powernv_led_match,
+ },
+};
+
+module_platform_driver(powernv_led_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("PowerNV LED driver");
+MODULE_AUTHOR("Vasant Hegde ");
--
Best Regards,
Jacek Anaszewski
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v6 3/3] leds/powernv: Add driver for PowerNV platform
Vasant,
On 21.07.2015 08:55, Vasant Hegde wrote:
On 07/21/2015 11:24 AM, Vasant Hegde wrote:
On 07/20/2015 03:10 AM, Jacek Anaszewski wrote:
Hi Vasant,
Jacek,
I've revised your patch and found few more issues.
Please refer to my comments below.
Thanks.
.../...
Please don't exceed 75 character line length limit.
Ok. I will fix it.. But I thought 80 character is the limit.
checkpatch.pl reports this.
Ah! I was running checkpatch.pl against source. Let me fix this.
.../...
+/*
+ * LED set routines have been implemented as work queue tasks scheduled
+ * on the global work queue. Individual task calls OPAL interface to set
+ * the LED state which might sleep for some time.
+ */
+struct powernv_led_data {
+struct led_classdevcdev;
+char*loc_code;/* LED location code */
+intled_type;/* OPAL_SLOT_LED_TYPE_* */
+enum led_brightnessvalue;/* Brightness value */
+struct mutexlock;
You're unnecessarily adding mutex for each LED class device.
The shared resource to protect is here powernv led interface,
so one mutex will suffice.
Ok. Let me move that to common structure.
+struct work_structwork_led;/* LED update workqueue */
+};
+
+struct powernv_leds_priv {
+int num_leds;
+struct powernv_led_data powernv_leds[];
+};
powernv_led_data doesn't have to be retained in the array. You access
the array elements only upon creation of LED class devices. When using
managed resource allocation you don't need to bother with freeing
resources, so you don't need to keep reference to the data.
I propose to drop struct powernv_leds_priv and instead introduce
a structure that would aggregate common driver data like mutex,
led_disable and max_led_type.
I still think we need two structures.. One for common driver data like mutex,
led_disable etc and other one for led data itself .. like
struct powernv_led_data {
struct led_classdev cdev;
char*loc_code; <-- pointer to DT node
int led_type; /* OPAL_SLOT_LED_TYPE_*
*/
enum led_brightness value; /* Brightness value */
};
struct powernv_led_common {
bool led_disable;
int max_led_type;
struct mutexlock;
};
Jacek,
Alternatively I can club both into single structure like below
struct powernv_led_data {
struct led_classdev cdev;
char*loc_code; <-- pointer to DT node
int led_type; /* OPAL_SLOT_LED_TYPE_*
*/
enum led_brightness value; /* Brightness value */
int *max_led_type;
struct mutex*lock;
};
static boolled_disable;
In this case I've to keep led_disable outside the structure as I need to access
this variable in powernv_led_remove().
OK, this looks good to me.
One remaining issue with these approach (where we don't have array of
powernv_led ) is,
powernv_let_set() function can sleep. Current code (v6) calls flush_work before
unloading
module. That way we are sure work is complete before unloading module.
With new approach, I'm not sure how I can make sure work is completed before
loading module.
led_classdev_unregister calls cancel_work_sync so brightness_set op
will not be called after driver detach.
> Does new workqueue approach has sleep functionality? Is there
way to make sure work is completed before
unloading module?
It is approached from the other side - we are making sure that
work will not be executed after driver detach.
--
Best Regards,
Jacek Anaszewski
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v6 3/3] leds/powernv: Add driver for PowerNV platform
Hi Vasant,
On 21.07.2015 07:54, Vasant Hegde wrote:
On 07/20/2015 03:10 AM, Jacek Anaszewski wrote:
Hi Vasant,
Jacek,
I've revised your patch and found few more issues.
Please refer to my comments below.
Thanks.
.../...
Please don't exceed 75 character line length limit.
Ok. I will fix it.. But I thought 80 character is the limit.
checkpatch.pl reports this.
Ah! I was running checkpatch.pl against source. Let me fix this.
.../...
+/*
+ * LED set routines have been implemented as work queue tasks scheduled
+ * on the global work queue. Individual task calls OPAL interface to set
+ * the LED state which might sleep for some time.
+ */
+struct powernv_led_data {
+struct led_classdevcdev;
+char*loc_code;/* LED location code */
+intled_type;/* OPAL_SLOT_LED_TYPE_* */
+enum led_brightnessvalue;/* Brightness value */
+struct mutexlock;
You're unnecessarily adding mutex for each LED class device.
The shared resource to protect is here powernv led interface,
so one mutex will suffice.
Ok. Let me move that to common structure.
+struct work_structwork_led;/* LED update workqueue */
+};
+
+struct powernv_leds_priv {
+int num_leds;
+struct powernv_led_data powernv_leds[];
+};
powernv_led_data doesn't have to be retained in the array. You access
the array elements only upon creation of LED class devices. When using
managed resource allocation you don't need to bother with freeing
resources, so you don't need to keep reference to the data.
I propose to drop struct powernv_leds_priv and instead introduce
a structure that would aggregate common driver data like mutex,
led_disable and max_led_type.
I still think we need two structures.. One for common driver data like mutex,
led_disable etc and other one for led data itself .. like
struct powernv_led_data {
struct led_classdev cdev;
char*loc_code; <-- pointer to DT node
int led_type; /* OPAL_SLOT_LED_TYPE_*
*/
enum led_brightness value; /* Brightness value */
};
struct powernv_led_common {
bool led_disable;
int max_led_type;
struct mutexlock;
};
This is exactly what I proposed. I didn't mention dropping
struct powernv_led_data.
+
+static __be64 max_led_type = cpu_to_be64(OPAL_SLOT_LED_TYPE_MAX);
The C standard prohibits initialization of global objects with non-constant
values. Section 6.7.8 of the C99 standard states:
"All the expressions in an initializer for an object that has static storage
duration shall be constant expressions or string literals."
Compilation succeeds when using powerpc64-linux-gcc because then
the following version of macro is used:
#define cpu_to_be64(x) (x)
and not
#define cpu_to_be64(x) swab64(x)
Please move max_led_type also to the struct powernv_leds_priv
and initialize it dynamically.
Yeah.. I should have added this to structure itself. Will fix.
+
+
+static inline int sizeof_powernv_leds_priv(int num_leds)
+{
+return sizeof(struct powernv_leds_priv) +
+(sizeof(struct powernv_led_data) * num_leds);
+}
+/* Returns OPAL_SLOT_LED_TYPE_* for given led type string */
+static int powernv_get_led_type(const char *led_type_desc)
+{
+int i;
+
+for (i = 0; i < ARRAY_SIZE(led_type_map); i++)
+if (!strcmp(led_type_map[i].desc, led_type_desc))
+return led_type_map[i].type;
+
+return -1;
+}
+
+/*
+ * This commits the state change of the requested LED through an OPAL call.
+ * This function is called from work queue task context when ever it gets
+ * scheduled. This function can sleep at opal_async_wait_response call.
+ */
+static void powernv_led_set(struct powernv_led_data *powernv_led)
+{
+int rc, token;
+u64 led_mask, led_value = 0;
+__be64 max_type;
+struct opal_msg msg;
+struct device *dev = powernv_led->cdev.dev;
+
+/* Prepare for the OPAL call */
+max_type = max_led_type;
+led_mask = OPAL_SLOT_LED_STATE_ON << powernv_led->led_type;
+if (powernv_led->value)
+led_value = led_mask;
+
+/* OPAL async call */
+token = opal_async_get_token_interruptible();
+if (token < 0) {
+if (token != -ERESTARTSYS)
+dev_err(dev, "%s: Couldn't get OPAL async token\n",
+__func__);
+return;
+}
+
+rc = opal_leds_set_ind(token, powernv_led->loc_code,
+ led_mask, led_value, &max_type);
+if (rc != OPAL_ASYNC_COMPLETION) {
+dev_err(dev, "%s: OPAL set LED call failed for %s [rc=%d]\n",
+__func__, powernv_led->loc_code, rc);
+goto out_token;
+}
+
+rc = opal_async_wait_response(token, &
Re: [PATCH v6 3/3] leds/powernv: Add driver for PowerNV platform
On 19.07.2015 23:40, Jacek Anaszewski wrote:
[...]
+/* Platform driver probe */
+static int powernv_led_probe(struct platform_device *pdev)
+{
+int num_leds;
+struct device_node *led_node;
+struct powernv_leds_priv *priv;
+
+led_node = of_find_node_by_path("/ibm,opal/leds");
+if (!led_node) {
+dev_err(&pdev->dev,
+"%s: LED parent device node not found\n", __func__);
+return -EINVAL;
+}
+
+num_leds = powernv_leds_count(led_node);
+if (num_leds <= 0) {
+dev_err(&pdev->dev,
+"%s: No location code found under LED node\n",
+__func__);
+return -EINVAL;
+}
You won't need to count the number of LEDs to register, just allocate
memory for each LED during parsing with managed resource allocation
API.
You can refer to drivers/leds/leds-bcm6328.c in order to gain
full picture on how this can look like.
struct powernv_led_data would have to carry the pointer to the
new common struct.
--
Best Regards,
Jacek Anaszewski
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v6 3/3] leds/powernv: Add driver for PowerNV platform
Hi Vasant,
I've revised your patch and found few more issues.
Please refer to my comments below.
On 17.07.2015 18:20, Vasant Hegde wrote:
On 07/17/2015 08:55 PM, Jacek Anaszewski wrote:
Hi Vasant,
Hi Jacek,
.../...
As per the LED class framework, the 'brightness_set' function should not
sleep. Hence these functions have been implemented through global work
queue tasks which might sleep on OPAL async call completion.
The platform level implementation of LED get and set state has been achieved
Please don't exceed 75 character line length limit.
Ok. I will fix it.. But I thought 80 character is the limit.
checkpatch.pl reports this.
through OPAL calls. These calls are made available for the driver by
exporting from architecture specific codes.
.../...
+
+#include
+
+/*
+ * By default unload path resets all the LEDs. But on PowerNV platform
+ * we want to retain LED state across reboot as these are controlled by
+ * firmware. Also service processor can modify the LEDs independent of
+ * OS. Hence avoid resetting LEDs in unload path.
+ */
+static bool led_disabled;
Please move this to the struct powernv_leds_priv.
Hmmm.. Ok. Will update and use platform_get_drvdata to access
platform_get_drvdata.
+
+/* Map LED type to description. */
+struct led_type_map {
+const inttype;
+const char*desc;
+};
+static const struct led_type_map led_type_map[] = {
+{OPAL_SLOT_LED_TYPE_ID,POWERNV_LED_TYPE_IDENTIFY},
+{OPAL_SLOT_LED_TYPE_FAULT,POWERNV_LED_TYPE_FAULT},
+{OPAL_SLOT_LED_TYPE_ATTN,POWERNV_LED_TYPE_ATTENTION},
+{-1,NULL},
+};
+
+/*
+ * LED set routines have been implemented as work queue tasks scheduled
+ * on the global work queue. Individual task calls OPAL interface to set
+ * the LED state which might sleep for some time.
+ */
+struct powernv_led_data {
+struct led_classdevcdev;
+char*loc_code;/* LED location code */
+intled_type;/* OPAL_SLOT_LED_TYPE_* */
+enum led_brightnessvalue;/* Brightness value */
+struct mutexlock;
You're unnecessarily adding mutex for each LED class device.
The shared resource to protect is here powernv led interface,
so one mutex will suffice.
+struct work_structwork_led;/* LED update workqueue */
+};
+
+struct powernv_leds_priv {
+int num_leds;
+struct powernv_led_data powernv_leds[];
+};
powernv_led_data doesn't have to be retained in the array. You access
the array elements only upon creation of LED class devices. When using
managed resource allocation you don't need to bother with freeing
resources, so you don't need to keep reference to the data.
I propose to drop struct powernv_leds_priv and instead introduce
a structure that would aggregate common driver data like mutex,
led_disable and max_led_type.
+
+static __be64 max_led_type = cpu_to_be64(OPAL_SLOT_LED_TYPE_MAX);
The C standard prohibits initialization of global objects with non-constant
values. Section 6.7.8 of the C99 standard states:
"All the expressions in an initializer for an object that has static storage
duration shall be constant expressions or string literals."
Compilation succeeds when using powerpc64-linux-gcc because then
the following version of macro is used:
#define cpu_to_be64(x) (x)
and not
#define cpu_to_be64(x) swab64(x)
Please move max_led_type also to the struct powernv_leds_priv
and initialize it dynamically.
Yeah.. I should have added this to structure itself. Will fix.
+
+
+static inline int sizeof_powernv_leds_priv(int num_leds)
+{
+return sizeof(struct powernv_leds_priv) +
+(sizeof(struct powernv_led_data) * num_leds);
+}
+/* Returns OPAL_SLOT_LED_TYPE_* for given led type string */
+static int powernv_get_led_type(const char *led_type_desc)
+{
+int i;
+
+for (i = 0; i < ARRAY_SIZE(led_type_map); i++)
+if (!strcmp(led_type_map[i].desc, led_type_desc))
+return led_type_map[i].type;
+
+return -1;
+}
+
+/*
+ * This commits the state change of the requested LED through an OPAL call.
+ * This function is called from work queue task context when ever it gets
+ * scheduled. This function can sleep at opal_async_wait_response call.
+ */
+static void powernv_led_set(struct powernv_led_data *powernv_led)
+{
+int rc, token;
+u64 led_mask, led_value = 0;
+__be64 max_type;
+struct opal_msg msg;
+struct device *dev = powernv_led->cdev.dev;
+
+/* Prepare for the OPAL call */
+max_type = max_led_type;
+led_mask = OPAL_SLOT_LED_STATE_ON << powernv_led->led_type;
+if (powernv_led->value)
+led_value = led_mask;
+
+/* OPAL async call */
+token = opal_async_get_token_interruptible();
+if (token < 0) {
+if (token != -ERESTARTSYS)
+dev_err(dev, "%s: Couldn't get OPAL async token\n",
+
Re: [PATCH v6 3/3] leds/powernv: Add driver for PowerNV platform
LED device tree node and register LED classdev structure */
+static int powernv_led_classdev(struct platform_device *pdev,
+ struct device_node *led_node,
+ struct powernv_leds_priv *priv, int num_leds)
+{
+ const char *cur = NULL;
+ int i, rc = -1;
+ struct property *p;
+ struct device_node *np;
+ struct powernv_led_data *powernv_led;
+ struct device *dev = &pdev->dev;
+
+ for_each_child_of_node(led_node, np) {
+ p = of_find_property(np, "led-types", NULL);
+ if (!p)
+ continue;
+
+ while ((cur = of_prop_next_string(p, cur)) != NULL) {
+ powernv_led = &priv->powernv_leds[priv->num_leds++];
+ if (priv->num_leds > num_leds) {
+ rc = -ENOMEM;
+ goto classdev_fail;
+ }
+ rc = powernv_led_create(dev,
+ powernv_led, np->name, cur);
+ if (rc)
+ goto classdev_fail;
+ } /* while end */
+ }
+
+ platform_set_drvdata(pdev, priv);
+ return rc;
+
+classdev_fail:
+ for (i = priv->num_leds - 2; i >= 0; i--) {
+ powernv_led = &priv->powernv_leds[i];
+ devm_led_classdev_unregister(dev, &powernv_led->cdev);
+ kfree(powernv_led->loc_code);
+ mutex_destroy(&powernv_led->lock);
+ }
+
+ return rc;
+}
+
+/*
+ * We want to populate LED device for each LED type. Hence we
+ * have to calculate count explicitly.
+ */
+static int powernv_leds_count(struct device_node *led_node)
+{
+ const char *cur = NULL;
+ int num_leds = 0;
+ struct property *p;
+ struct device_node *np;
+
+ for_each_child_of_node(led_node, np) {
+ p = of_find_property(np, "led-types", NULL);
+ if (!p)
+ continue;
+
+ while ((cur = of_prop_next_string(p, cur)) != NULL)
+ num_leds++;
+ }
+
+ return num_leds;
+}
+
+/* Platform driver probe */
+static int powernv_led_probe(struct platform_device *pdev)
+{
+ int num_leds;
+ struct device_node *led_node;
+ struct powernv_leds_priv *priv;
+
+ led_node = of_find_node_by_path("/ibm,opal/leds");
+ if (!led_node) {
+ dev_err(&pdev->dev,
+ "%s: LED parent device node not found\n", __func__);
+ return -EINVAL;
+ }
+
+ num_leds = powernv_leds_count(led_node);
+ if (num_leds <= 0) {
+ dev_err(&pdev->dev,
+ "%s: No location code found under LED node\n",
+ __func__);
+ return -EINVAL;
+ }
+
+ priv = devm_kzalloc(&pdev->dev,
+ sizeof_powernv_leds_priv(num_leds), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ return powernv_led_classdev(pdev, led_node, priv, num_leds);
+}
+
+/* Platform driver remove */
+static int powernv_led_remove(struct platform_device *pdev)
+{
+ int i;
+ struct powernv_led_data *powernv_led;
+ struct powernv_leds_priv *priv;
+
+ /* Disable LED operation */
+ led_disabled = true;
+
+ priv = platform_get_drvdata(pdev);
+
+ for (i = 0; i < priv->num_leds; i++) {
+ powernv_led = &priv->powernv_leds[i];
+ devm_led_classdev_unregister(&pdev->dev, &powernv_led->cdev);
+ flush_work(&powernv_led->work_led);
+ kfree(powernv_led->loc_code);
+ mutex_destroy(&powernv_led->lock);
+ }
+
+ dev_info(&pdev->dev, "PowerNV led module unregistered\n");
+ return 0;
+}
+
+/* Platform driver property match */
+static const struct of_device_id powernv_led_match[] = {
+ {
+ .compatible = "ibm,opal-v3-led",
+ },
+ {},
+};
+MODULE_DEVICE_TABLE(of, powernv_led_match);
+
+static struct platform_driver powernv_led_driver = {
+ .probe = powernv_led_probe,
+ .remove = powernv_led_remove,
+ .driver = {
+ .name = "powernv-led-driver",
+ .owner = THIS_MODULE,
+ .of_match_table = powernv_led_match,
+ },
+};
+
+module_platform_driver(powernv_led_driver);
+
+MODULE_LICENSE("GPL");
If you want to be consistent with what you declare in the beginnig
then it should be:
MODULE_LICENSE("GPL v2")
+MODULE_DESCRIPTION("PowerNV LED driver");
+MODULE_AUTHOR("Vasant Hegde ");
--
Best Regards,
Jacek Anaszewski
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v5 3/3] leds/powernv: Add driver for PowerNV platform
Hi Vasan,
On 07/16/2015 08:54 AM, Vasant Hegde wrote:
On 07/14/2015 02:30 PM, Jacek Anaszewski wrote:
Hi Vasant,
Jacek,
Thanks for the update. I think that we have still room
for improvements, please look at my comments below.
Thanks for the detailed review.
You're welcome.
.../...
@@ -0,0 +1,24 @@
+Device Tree binding for LEDs on IBM Power Systems
+-
+
Please start with:
-
Required properties:
- compatible : Should be "ibm,opal-v3-led".
Each location code of FRU/Enclosure must be expressed in the
form of a sub-node.
Required properties for the sub nodes:
- led-types : Supported LED types (attention/identify/fault) provided
in the form of string array.
-
or something of this flavour. The example should be at the end.
Fixed.
+The 'leds' node under '/ibm,opal' lists service indicators available in the
+system and their capabilities.
+
+leds {
+compatible = "ibm,opal-v3-led";
+led-mode = "lightpath";
What about led-mode property? If it is generated by firmware I think,
that this should be mentioned somehow.
Yes.. Its generated by firmware. Added this property to documentation file.
+
+U78C9.001.RST0027-P1-C1 {
+led-types = "identify", "fault";
+};
+...
+...
+};
+
+Each node under 'leds' node describes location code of FRU/Enclosure.
+
+compatible : should be : "ibm,opal-v3-led".
Second colon was redundant here.
I have added as
- compatible : "ibm,opal-v3-led".
Please retain "Should be :".
+
+The properties under each node:
+
+ led-types : Supported LED types (attention/identify/fault).
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index 4191614..4f56c7a 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -505,6 +505,17 @@ config LEDS_BLINKM
This option enables support for the BlinkM RGB LED connected
through I2C. Say Y to enable support for the BlinkM LED.
+config LEDS_POWERNV
+tristate "LED support for PowerNV Platform"
+depends on LEDS_CLASS
+depends on PPC_POWERNV
+depends on OF
+help
+ This option enables support for the system LEDs present on
+ PowerNV platforms. Say 'y' to enable this support in kernel.
+ To compile this driver as a module, choose 'm' here: the module
+ will be called leds-powernv.
+
config LEDS_SYSCON
bool "LED support for LEDs on system controllers"
depends on LEDS_CLASS=y
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index bf46093..480814a 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -59,6 +59,7 @@ obj-$(CONFIG_LEDS_SYSCON)+= leds-syscon.o
obj-$(CONFIG_LEDS_VERSATILE)+= leds-versatile.o
obj-$(CONFIG_LEDS_MENF21BMC)+= leds-menf21bmc.o
obj-$(CONFIG_LEDS_PM8941_WLED)+= leds-pm8941-wled.o
+obj-$(CONFIG_LEDS_POWERNV)+= leds-powernv.o
# LED SPI Drivers
obj-$(CONFIG_LEDS_DAC124S085)+= leds-dac124s085.o
diff --git a/drivers/leds/leds-powernv.c b/drivers/leds/leds-powernv.c
new file mode 100644
index 000..b5a307c
--- /dev/null
+++ b/drivers/leds/leds-powernv.c
@@ -0,0 +1,463 @@
+/*
+ * PowerNV LED Driver
+ *
+ * Copyright IBM Corp. 2015
+ *
+ * Author: Vasant Hegde
+ * Author: Anshuman Khandual
+ *
+ * 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.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+
+/*
+ * By default unload path resets all the LEDs. But on PowerNV platform
+ * we want to retain LED state across reboot as these are controlled by
+ * firmware. Also service processor can modify the LEDs independent of
+ * OS. Hence avoid resetting LEDs in unload path.
+ */
+static bool led_disabled;
+
+/* Map LED type to description. */
+struct led_type_map {
+const inttype;
+const char*desc;
+};
+static const struct led_type_map led_type_map[] = {
+{OPAL_SLOT_LED_TYPE_ID,POWERNV_LED_TYPE_IDENTIFY},
+{OPAL_SLOT_LED_TYPE_FAULT,POWERNV_LED_TYPE_FAULT},
+{OPAL_SLOT_LED_TYPE_ATTN,POWERNV_LED_TYPE_ATTENTION},
+{-1,NULL},
+};
+
+/*
+ * LED set routines have been implemented as work queue tasks scheduled
+ * on the global work queue. Individual task calls OPAL interface to set
+ * the LED state which might sleep for some time.
+ */
+struct powernv_led_data {
+struct led_classdevcdev;
+enum led_brightnessvalue; /* Brightness value */
+struct mutexlock;
+struct work_structwork_led; /* LED update workqueue */
+};
+
+struct powernv_leds_priv {
+
Re: [PATCH v5 3/3] leds/powernv: Add driver for PowerNV platform
powernv_led = &priv->powernv_leds[i];
+ powernv_led_delete(powernv_led);
+ flush_work(&powernv_led->work_led);
+ }
You are missing mutex_destroy here.
+
+ dev_info(&pdev->dev, "PowerNV led module unregistered\n");
+ return 0;
+}
+
+/* Platform driver property match */
+static const struct of_device_id powernv_led_match[] = {
+ {
+ .compatible = "ibm,opal-v3-led",
+ },
+ {},
+};
+MODULE_DEVICE_TABLE(of, powernv_led_match);
+
+static struct platform_driver powernv_led_driver = {
+ .probe = powernv_led_probe,
+ .remove = powernv_led_remove,
+ .driver = {
+ .name = "powernv-led-driver",
+ .owner = THIS_MODULE,
+ .of_match_table = powernv_led_match,
+ },
+};
+
+module_platform_driver(powernv_led_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("PowerNV LED driver");
+MODULE_AUTHOR("Vasant Hegde ");
--
Best Regards,
Jacek Anaszewski
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v4 0/3] LED interface for PowerNV platform
On 06/11/2015 08:33 AM, Vasant Hegde wrote: On 04/28/2015 03:39 PM, Vasant Hegde wrote: The following series implements LED driver for PowerNV platform. Ben, Michael, Can you please review/ACK this patchset? Yeah, we are waiting for ppc maintainer's ack, also to clarify DT related issues. Ben himself asked to wait for the ack in the message [1]. We are waiting for almost two months now :) [1] http://www.spinics.net/lists/linux-leds/msg03557.html -- Best Regards, Jacek Anaszewski ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v4 3/3] leds/powernv: Add driver for PowerNV platform
v_err(&pdev->dev,
+ "%s: No location code found under LED node\n",
+ __func__);
+ return -EINVAL;
+ }
+
+ priv = devm_kzalloc(&pdev->dev,
+ sizeof_powernv_leds_priv(num_leds),
+ GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ return powernv_led_classdev(pdev, led_node, priv, num_leds);
+}
+
+/* Platform driver remove */
+static int powernv_led_remove(struct platform_device *pdev)
+{
+ int i;
+ struct powernv_led_data *powernv_led;
+ struct powernv_leds_priv *priv;
+
+ /* Disable LED operation */
+ led_disabled = true;
+
+ priv = platform_get_drvdata(pdev);
+
+ for (i = 0; i < priv->num_leds; i++) {
+ powernv_led = &priv->powernv_leds[i];
+ powernv_led_delete(powernv_led);
+ flush_work(&powernv_led->work_led);
+ }
+
+ dev_info(&pdev->dev, "PowerNV led module unregistered\n");
+ return 0;
+}
+
+/* Platform driver property match */
+static const struct of_device_id powernv_led_match[] = {
+ {
+ .compatible = "ibm,opal-v3-led",
+ },
+ {},
+};
+MODULE_DEVICE_TABLE(of, powernv_led_match);
+
+static struct platform_driver powernv_led_driver = {
+ .probe = powernv_led_probe,
+ .remove = powernv_led_remove,
+ .driver = {
+ .name = "powernv-led-driver",
+ .owner = THIS_MODULE,
+ .of_match_table = powernv_led_match,
+ },
+};
+
+module_platform_driver(powernv_led_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("PowerNV LED driver");
+MODULE_AUTHOR("Vasant Hegde ");
--
To unsubscribe from this list: send the line "unsubscribe linux-leds" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Best Regards,
Jacek Anaszewski
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
On 04/28/2015 08:59 AM, Stewart Smith wrote: Jacek Anaszewski writes: Is the DT node we are discussing used by some other drivers than the LED class driver? Or is it required in this form by other components of your platform? OS kernels are the chief consumers, Linux being the overwhelmingly major one here. But this is what firmware currently produces. Changing the DT representation at this stage is perhaps *possible* without creating a bunch of pain (I'd have to audit a bunch of things to see if we have GA shipping systems with this functionality for instance, and then evaluate the impact to partners and our various labs) which is a lot of work I don't particularly want to do and is well below urgent item 248 on my TODO list, especially for what seems to be largely a cosmetic suggestion? That being said, more and better review of things we're putting in the device tree in firmware is probably a good thing. After all, once we release we do kind of have to live with it essentially forever. If people are able to aid in that kind of code review, I'd be most welcome to hear it. -- To unsubscribe from this list: send the line "unsubscribe linux-leds" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html As we agreed with Ben - DT bindings are platform specific and they will have to be acked by powerpc maintainers. I have only DT related experience from ARM based platforms, but it seems not to be applicable to powerpc. -- Best Regards, Jacek Anaszewski ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
On 04/27/2015 11:53 AM, Benjamin Herrenschmidt wrote: On Mon, 2015-04-27 at 09:24 +0200, Jacek Anaszewski wrote: I was not aware that some other entity than the driver could be interested in the information provided by DT node. I will no longer object, provided that we will get an ack from DT maintainer. My understanding is that we no longer need bindings to be accepted by DT maintainers. I've just skimmed through the patches in Documentation/devicetree/bindings/powerpc and they indeed don't have acks from DT maintainers. That being said, I would like to review this one before it is accepted, as I haven't had a chance to do so yet :-) (I was mostly responding to you, I haven't caught up with the full thread yet). Please wait for an ack from myself or Michael Ellerman (the two powerpc maintainers). Either that or provide your ack and we'll merge it via our tree if we are happy with it. Since this driver is for LED subsystem it should better go via leds tree. -- Best Regards, Jacek Anaszewski ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
Hi Vasant,
On 04/24/2015 12:15 PM, Jacek Anaszewski wrote:
[...]
For attention and fault LEDs only brightness attribute would matter.
Sure.
DT bindings would look as follows:
opal-leds {
compatible = "ibm,opal-leds";
U78C9.001.RST0027-P1-C1:fault {
};
U78C9.001.RST0027-P1-C1:indent {
};
U78C9.001.RST0027-P2-C1:attn
};
}
}
As mentioned earlier DT is coming from our firmware. For now I will
respin another round of patches by using led-types property and run
it through DT experts (DT mailing list). If they insist this method
is better than what I already have , then will work with my firmware
folks to see what we can do better.
Please hold on with sending the patches until we clarify all the
DT related issues. Having full picture will help to adjust the
bindings better to LED common bindings specification.
Please send the next version adjusted to your platform needs.
Since I've found out that only powerpc maintainers will have to
ack DT patch, you can avoid adding devicetree list on cc.
--
Best Regards,
Jacek Anaszewski
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
Hi Benjamin, Thanks for your explanation. On 04/27/2015 12:07 AM, Benjamin Herrenschmidt wrote: On Thu, 2015-04-23 at 16:13 +0200, Jacek Anaszewski wrote: How the firmware is related to kernel? These bindings are for kernel, not for the firmware. There should be no relation. DT bindings should be kernel agnostic and represent the HW layout, not be designed based on what a given kernel driver wants, but I'm digressing... DT bindings are compiled to *.dtb file which is concatenated with zImage. No. First, a "binding" isn't compiled to a dtb, a binding is a piece documentation. A flat device tree *might* be compiled from a dts to a dtb but that isn't necessarily the case. For example, any machine with an actual implementation of Open Firmware will essentially flatten OF internal tree into a dtb at boot time, and that tree is itself generated by forth code. In our case, the device tree is procedurally generated by two layers of firmware, there is no dts "compilation" happening. HostBoot generates a shell and OPAL extends it before flattening it and passing it to the kernel. The "concatenated with zImage" point you make is also a very ARM centric one. ARM provides the *optional* ability to concatenate a dtb with a zImage, but that's specific to ARM zImage wrapper. For example, powerpc can do something similar (but not identical) using the "wrapper" script we have in arch/powerpc/boot where we embed the dtb. However, this too is optional, we have a longer history of having firwmares generating device-trees. Note: We invented the whole FDT business :-) During system boot device drivers are matched with DT bindings through 'compatible' property. A driver should have single matching DT node, i.e. no other driver can probe with the same DT node. This implies that the node should contain only the properties required for configuring the related device. I don't see how you goes from A to implying B here. Yes, a device generally will have a single representing node but that doesn't mean that the node only contains what the driver needs. The DT node can contain all sort of auxilliary informations that the driver may or may not be interested in that was deemed potentially relevant or useful by the platform designer. I was not aware that some other entity than the driver could be interested in the information provided by DT node. I will no longer object, provided that we will get an ack from DT maintainer. -- Best Regards, Jacek Anaszewski ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
On Fri, 24 Apr 2015 14:18:30 +1000 Stewart Smith wrote: > Jacek Anaszewski writes: > >> These device tree comes from out firmware ... which is immutable . > > > > How the firmware is related to kernel? These bindings are for > > kernel, not for the firmware. > > > > DT bindings are compiled to *.dtb file which is concatenated with > > zImage. During system boot device drivers are matched with DT > > bindings through 'compatible' property. A driver should have single > > matching DT node, i.e. no other driver can probe with the same DT > > node. This implies that the node should contain only the properties > > required for configuring the related device. > > For OPAL firmware on POWER, firmware hands kernel a flattened device > tree of the machine it's booting on. It's not added to kernel as the > kernels aren't board specific - they're generic. Is the DT node we are discussing used by some other drivers than the LED class driver? Or is it required in this form by other components of your platform? > https://github.com/open-power/skiboot/ is the firmware that generates > the device tree for booting under OPAL. > -- Best Regards, Jacek Anaszewski ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
On Fri, 24 Apr 2015 11:00:41 +0530 Hi Vasant, Vasant Hegde wrote: > On 04/23/2015 07:43 PM, Jacek Anaszewski wrote: > > On Thu, 23 Apr 2015 10:55:40 +0530 > > Vasant Hegde wrote: > > > > Hi Jacek, > > .../... > > >> > >> These device tree comes from out firmware ... which is immutable . > > > > How the firmware is related to kernel? These bindings are for > > kernel, not for the firmware. > > > > DT bindings are compiled to *.dtb file which is concatenated with > > zImage. During system boot device drivers are matched with DT > > bindings through 'compatible' property. A driver should have single > > matching DT node, i.e. no other driver can probe with the same DT > > node. This implies that the node should contain only the properties > > required for configuring the related device. > > > > As Stewart mentioned, its not .dtb file in our case.. we pass > flattened device tree .. which is built by OPAL. No matter what format of device tree OPAL produces, I assume that it must compile it from some sources. dtb file is a compiled form of human readable dts file containing Flattened Device Tree - a data structure for describing the hardware in the system. Please refer to: http://elinux.org/Device_Tree > >> We can use LED node name + led-type property for naming...which is > >> what I do currently (v4.. which I haven't posted) > >> > >> > >>> 1. Each LED would have one corresponding LED class device. > >>> > >>> 2. Operations on attn and fault LED types: > >>> turn on: > >>> echo 255 > brightness > >>> turn off: > >>> echo 0 > brightness > >>> get status > >>> cat brightness > >>> > >>> 3. Operations on identify LED: > >>> turn on: > >>> echo "timer" > trigger > >>> (blink_set op would have to be implemented in the > >>> driver) > >>> turn off: > >>> echo 0 > brightness > >>> get status: > >>> support for this would have to be added to the LED > >>> subsystem core > >> > >> I see few issues here. > >> - Overloading same LED device with multiple opeartion complicates > >> things .. as these operations can be done independently (say user > >> is allowed to enable both identify and fault simultaneously) > > > > I agree, it would be hard to distinguish whether by executing > > `echo 0 > brightness` we want to turn off identify or fault > > function. > > > >> - point 3: IIUC after duration value expires identify indicator > >> reverts.. we don't want to revert until user asks . > > > > From what you shared, blinking has hardware acceleration on OPAL > > side. At first timer trigger tries to use HW accelerated blinking by > > calling blink_set op and resorts to using software fallback only if > > the op fails or is not defined. > > Blinking is the physical state of LED to represent "identify" state. > which is taken care by hardware. and OS doesn't have control on > this .. I am aware of it. Therefore we would probably need to add a flag LED_BLINK_HW_ONLY to the LED subsystem core and modify led_blink_set function to log an error and avoid setting software fallback in case blink_set op fails and the flag is set. Nevertheless, I am leaning towards using brightness_set op for this. > From software point of view its just another LED with two state (ON > and OFF). > > > > > BTW timer trigger re-sets blink after timer expires, unless > > LED_BLINK_ONESHOT flag is set by LED class device. > > In my case, I want to retain the state.' > > > > >> - point 3: if I use brightness for both identify/fault, how to > >> disable these LEDs independently? > > > > Another sysfs attribute would be required, but it would be ugly. > > yeah. > > > >> - Also how to use trigger property for each LED (if at all we > >> want to use them later)? > >> > > > > After analyzing pros and cons I think that separate LED class > > devices for each LED type would be most suitable solution in this > > case. > > Agree. > > > > > For 'identify' LED the operation would be: > > > > #echo "timer" > trigger //set 'identify' (blinking) > > #cat trigger//check "identify" sta
Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
On Thu, 23 Apr 2015 10:55:40 +0530
Vasant Hegde wrote:
> On 04/23/2015 03:15 AM, Jacek Anaszewski wrote:
> > Hi Vasant,
> >
>
> Hi Jacek,
>
>
> .../...
>
> >>
> >>>
> >>> From what I can see from the driver code the LEDs are set with:
> >>>
> >>> opal_leds_set_ind(token, loc_code, led_mask, led_value,
> >>> &max_led_type);
> >>>
> >>> and their state can be read with:
> >>>
> >>> opal_leds_get_ind(loc_code, &led_mask, &led_value, &max_led_type)
> >>>
> >>> From the kernel point of view these are very simple operations.
> >>>
> >>> All the logic you described should be handled by user space.
> >>> If you need to be able to specify the LED mode you want to
> >>> set/read, then additional 'mode' sysfs attribute should be added
> >>> by the driver. There would have to be also additional sysfs
> >>> attribute 'available_modes" provided. The ABI documentation
> >>> should inform how the mode identifiers map to the modes. I
> >>> already explained how to add it, when we were discussing about
> >>> retaining led state on remove.
> >>
> >> Sorry..My fault.. I should have elaborated mode operation...
> >
> > What I was thinking about here was actually LED type, not mode in
> > terms of Guiding Light/Light Path. However, please look at the
> > newest approach in the end of this message.
> >
>
> No problem.
>
> >> I forgot to mention that LED mode is static... meaning platform
> >> provides this information, but we cannot change during runtime..
> >>
> >> Presently we have this information in Device Tree. Since this is
> >> static one (and also LED Mode is system wide.. nothing to do
> >> individual LED), I didn't add it in LED driver code.. .Do you
> >> think we should add that property ?
> >
> > The property shouldn't be documented at all if it isn't to be used.
>
> Ok . I will remove this.
>
> >
> >>>
> >>>
> >>> I'd see following use cases.
> >>>
> >>> (let's assume that modes are defined as follows:
> >>> 0: ident, 1: attn, 2: fault)
> >>
> >> Modes are : Guiding Light / Light Path ... which is static and
> >> platform provides this information.
> >>
> >> LED types : IDENT, FAULT and ATTN which can be get/set/reset
> >> by OS (kernel/userspace)
> >>
> >> Also only 1 LED can be ATTN ...
> >>
> >>> #cat available_modes
> >>> #0 1 2
> >>> #echo 0 > mode //set ident mode
> >>> #echo 1 > brightness //set ident state
> >>> #echo 2 > mode //set fault mode
> >>> #cat brightness //read fault state
> >>> #0
> >>> #echo 1 > attn //set attn mode
> >>> #echo 1 > brightness
> >>>
> >>> This would set the LED in blinking mode, so I am wondering if we
> >>> shouldn't employ timer trigger for this to keep the LED API
> >>> consistent.
> >>>
> >>> Can a single LED support other mode than 'attention'? I'd like to
> >>> know if a LED in attention mode (blinking), can be set to some
> >>> solid mode?
> >>
> >> No.. Its always single attention LED/system ... which can be Set
> >> (Solid) / reset state.
> >
> > I confused it with ident.
>
> No problem. We have many hardware specific jargon's which is enough
> to confuse anyone :-)
>
> >
> >>>
> >>> Please let me know if such an approach would still not fit for
> >>> your requirements.
> >>>
> >>
> >> Given above conditions, I think current approach (my v3 patchset)
> >> is simple and works better. What you say?
> >
> > Yes, but we still have naming and blinking issues to solve.
> >
> > Please look at this draft design of device tree node:
> >
> > opal-leds {
> > compatible = "ibm,opal-v3-led";
> >
> > U78C9.001.RST0027-P1-C1:attn {
> > };
> >
> > U78C9.001.RST0027-P2-C1:identify:fault
> > };
> >
> > U78C9.001.RST0027-P3-C2:identify:fault {
> > };
> > };
> > };
> >
> > The LED nodes could be empty as the name would convey all the
> > required informat
Re: [PATCH v3 3/3] leds/powernv: Add driver for PowerNV platform
> On 21.04.2015 07:50, Vasant Hegde wrote:
> On 04/20/2015 08:57 PM, Jacek Anaszewski wrote:
>> Hi Vasant,
>>
>
> Jacek,
>
> Thanks for the review.
You are welcome.
>> Thanks for the update.
>>
>> On 04/20/2015 10:01 AM, Vasant Hegde wrote:
>>> This patch implements LED driver for PowerNV platform using the
>>> existing generic LED class framework.
>>>
>>> PowerNV platform has below type of LEDs:
>>> - System attention
>>> Indicates there is a problem with the system that needs
>>> attention.
>>> - Identify
>>> Helps the user locate/identify a particular FRU or resource
>>> in the system.
>>> - Fault
>>> Indicates there is a problem with the FRU or resource at the
>>> location with which the indicator is associated.
>>>
>>> We registers classdev structures for all individual LEDs detected
>>> on the
>>
>> s/registers/register
>
> Fixed.
>
>>
>>> system through LED specific device tree nodes. Device tree nodes
>>> specify what all kind of LEDs present on the same location code.It
>>> registers LED classdev structure for each of them.
>>>
>>> All the system LEDs can be found in the same regular
>>> path /sys/class/leds/. We don't use LED colors. Hence our LEDs have
>>> names in this format.
>>>
>>> :
>>
>> I think that powernv prefix should be also present in the beginning.
>> What would be the format of ? Does it identify a LED
>> uniquely.
>
> Location code is unique and its guaranteed that we don't clash here.
> Also this is platform specific LEDs and I don't see any value by
> prefixing "powernv". Hence I think its fine with current format.
>
> Sample location code : U78C9.001.RST0027-P1-C1
>Here "U78C9.001.RST0027" represents the system model
> P1 - Planar 1
> C1 - Card 1
I consulted ePAPR standard and all the characters used for location code
are allowed. Nonetheless AFAIK no existing bindings use uppercase for
node name.
We will need DT maintainer's ack here too, so cc DT list.
Please CC devicet...@vger.kernel.org next time, especially for
DT documentation patch.
> .../...
>
>>> +led {
>>> +compatible = "ibm,opal-v3-led";
>>> +phandle = <0x106b>;
>>> +linux,phandle = <0x106b>;
>>> +led-mode = "lightpath";
>>> +
>>> +U78C9.001.RST0027-P1-C1 {
>>
>> DT node names should be in lowercase form.
>> Is U78C9.001.RST0027-P1-C1 a location code?
>
> Yes ... Node presents the location code.. Platform provides these
> location code in upper case. Hence our OPAL firmware exposes them in
> upper case.
>
>>
>>> +led-types = "identify", "fault";
>>> +led-loc = "descendent";
>>> +phandle = <0x106f>;
>>> +linux,phandle = <0x106f>;
I think that all these properties will not be needed.
>>> +};
>>> +...
>>> +...
>>> +};
>>> +
>>> +
>>> +'compatible' property describes LEDs compatibility.
>>
>> compatible doesn't need to be mentioned here.
>
> Removed.
>
>>
>>> +'led-mode' property describes service indicator mode
>>> (lightpath/guidinglight).
>>
>> You don't parse this in the driver? What is its purpose?
>
> As mentioned in other thread, currently our driver doesn't parse
> this. (As mode is provided by platform and its static).
>
> Do you want me to elaborate this property here?
I don't think so. At least for now :)
> .../...
>
>>> +
>>> +/*
>>> + * By default unload path resets all the LEDs. But on PowerNV
>>> platform
>>> + * we want to retain LED state across reboot as these are
>>> controlled by
>>> + * firmware. Also service processor can modify the LEDs
>>> independent of
>>> + * OS. Hence avoid resetting LEDs in unload path.
>>> + */
>>> +static bool led_disabled;
>>
>> I think that we have to make it configurable from the Device Tree
>> level. There could be a 'retain-state-removed' property introduced.
>
> Hmmm.. In my case, LED behavior is not going to change. Hence I don't
> think we need this property.
Can you guarantee that the next version of the platform will have also
the same requirements? What if
Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
Hi Vasant, On 20.04.2015 17:53, Vasant Hegde wrote: > On 04/20/2015 08:50 PM, Jacek Anaszewski wrote: >> On 04/20/2015 02:34 PM, Vasant Hegde wrote: >>> On 04/20/2015 05:15 PM, Jacek Anaszewski wrote: >>>> Hi Vasant, >>>> >>>> I'd like to clarify some details regarding your explanation. >>>> >>>> On 04/15/2015 12:15 PM, Vasant Hegde wrote: >>>> [...] >>>>>>> >>>>>>> In Power Systems LEDs are overloaded (meaning same LED is used >>>>>>> for identify and >>>>>>> fault depending on their state --- blinking = identify and >>>>>>> solid = fault). Hence here append LED type info. >>>>>> >>>>>> The label could be composed of segments and an ordinal number as >>>>>> labels have to be unique, e.g. attn_ident_0, attn_ident_1. >>>>>> The segments would have to be parsed by the driver to discover >>>>>> all the LED's available modes. >>>>>> >>>>>> nitpicking: identify is a verb and is not a proper name for the >>>>>> LED. Could you describe the purpose of this mode, so that we >>>>>> could come up with a better name? >>>>> >>>>> Each component (Field Replacement Unit) will have service >>>>> indicator (LEDS) which >>>>> can have below states : >>>>> - OFF : no action >>>>> - Identify: blinking state (user can use this state to >>>>> identify particular component). >>>>> In Power Systems world we call it as "identify" >>>>> indicator.. Hence I retained same name here. >>>>> How about just "ident" ? >>>>> - fault : solid state (when component goes bad, LED goes to >>>>> solid state) Note that our FW is capable of isolating some of the >>>>> issues and it can turn >>>>> on LEDs without OS >>>>> interference. >>>>> >>>>> We have one more System level LED (System Attention Indicator).. >>>>> This LED has two states: >>>>> - OFF : Everything is fine >>>>> - ON : Some component has issues and needs attention. >>>> >>>> We have three modes: >>>> - identify- blinking >>>> - fault- solid >>>> - attention indicator- solid >>>> >>>> How does LED operation differ for fault and attention modes? >>>> Does a LED have different intensity? >>> >>> Jacek, >>> >>> System Attention LED is special LED and its single LED >>> available/system. where as identify and fault is applicable to all >>> field replaceable units in the system.. >>> >>> So Typical server will have >>> 1 System Attention LED >>> N Identify/fault LED (N = Field Replaceable Unit). >>> >>> Apart from above two, we do have two more LEDs/Enclosure (external >>> visible LEDs) >>> - Enclosure Identify >>> - Enclosure fault >>> These LEDs reflects state of all Field Replaceable Units >>> (FRU) inside this enclosure >>> If any of FRU state is ON, this will become ON >>> Also we can independently enable this LED!! >>> >>> But from kernel side implementation point of view, I just >>> treat this as another LED.. as our platform code (OPAL firmware) >>> takes care of roll up etc. >>> >>> >>> Now our LED can operate in two mode (Depending on our service >>> model, typically one/two socket servers are Light Path mode, >>> whereas high end servers are Guiding Light Mode). >>> >>>1. Guiding Light >>>Only Identify indicator is support.. Fault is not supported >>>System attention indicator is used to point there is some >>> problem in system and need attention >>> 2. Light Path mode >>> Both identify and fault indicator is supported .. >>> Fault is ON whenever some component is faulty >>> System attention indicator is used to point that FW/OS is not >>> able to isolate the problem and needs user to look into serviceable >>> event (like syslog/ our agents like ESA which analyzes and reports >>> events) >>> >>> >>> Handling LED states : >>>
Re: [PATCH v3 3/3] leds/powernv: Add driver for PowerNV platform
zalloc
and it will be released on remove automatically.
+ dev_dbg(&pdev->dev,
+ "Classdev registration successful for %s\n",
+ powernv_led->cdev.name);
+ }
+ return rc;
+}
+
+/* Platform driver probe */
+static int powernv_led_probe(struct platform_device *pdev)
+{
+ int rc = 0;
+ struct device_node *led_node, *np;
+
+ led_node = of_find_node_by_path("/ibm,opal/led");
+ if (!led_node) {
+ dev_err(&pdev->dev,
+ "%s: LED parent device node not found\n", __func__);
+ return -EINVAL;
+ }
+
+ for_each_child_of_node(led_node, np) {
+ if (has_led_type(np, LED_TYPE_ATTENTION))
+ rc = power_led_classdev(pdev, np,
+ OPAL_SLOT_LED_TYPE_ATTN);
+
+ if (has_led_type(np, LED_TYPE_IDENTIFY))
+ rc = power_led_classdev(pdev, np,
+ OPAL_SLOT_LED_TYPE_ID);
+
+ if (has_led_type(np, LED_TYPE_FAULT))
+ rc = power_led_classdev(pdev, np,
+ OPAL_SLOT_LED_TYPE_FAULT);
+ }
+ return rc;
+}
+
+/* Platform driver remove */
+static int powernv_led_remove(struct platform_device *pdev)
+{
+ struct powernv_led_data *powernv_led;
+
+ /* Disable LED operation */
+ led_disabled = true;
+
+ dev_dbg(&pdev->dev,
+ "%s: Unregister all classdev structures\n", __func__);
+ list_for_each_entry(powernv_led, &powernv_led_list, link)
+ led_classdev_unregister(&powernv_led->cdev);
+
+ dev_dbg(&pdev->dev,
+ "%s: Wait for all work tasks to finish\n", __func__);
+ list_for_each_entry(powernv_led, &powernv_led_list, link)
+ flush_work(&powernv_led->work_led);
+
+ while (!list_empty(&powernv_led_list)) {
+ powernv_led = list_first_entry(&powernv_led_list,
+ struct powernv_led_data, link);
+ list_del(&powernv_led->link);
+ kfree(powernv_led);
+ }
+
+ dev_info(&pdev->dev, "PowerNV led module unregistered\n");
+ return 0;
+}
+
+/* Platform driver property match */
+static const struct of_device_id powernv_led_match[] = {
+ {
+ .compatible = "ibm,opal-v3-led",
+ },
+ {},
+};
+MODULE_DEVICE_TABLE(of, powernv_led_match);
+
+static struct platform_driver powernv_led_driver = {
+ .probe = powernv_led_probe,
+ .remove = powernv_led_remove,
+ .driver = {
+ .name = "powernv-led-driver",
+ .owner = THIS_MODULE,
+ .of_match_table = powernv_led_match,
+ },
+};
+
+module_platform_driver(powernv_led_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("PowerNV LED driver");
+MODULE_AUTHOR("Vasant Hegde ");
--
Best Regards,
Jacek Anaszewski
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v3 1/3] powerpc/powernv: Add OPAL interfaces for accessing and modifying system LED states
Hi Vasant, Anshuman,
Thanks for the update.
On 04/20/2015 09:59 AM, Vasant Hegde wrote:
From: Anshuman Khandual
This patch registers the following two new OPAL interfaces calls
for the platform LED subsystem. With the help of these new OPAL calls,
the kernel will be able to get or set the state of various individual
LEDs on the system at any given location code which is passed through
the LED specific device tree nodes.
(1) OPAL_LEDS_GET_INDICATOR opal_leds_get_ind
(2) OPAL_LEDS_SET_INDICATOR opal_leds_set_ind
Signed-off-by: Anshuman Khandual
Signed-off-by: Vasant Hegde
Acked-by: Stewart Smith
Tested-by: Stewart Smith
---
arch/powerpc/include/asm/opal-api.h| 29 +++-
arch/powerpc/include/asm/opal.h|5
arch/powerpc/platforms/powernv/opal-wrappers.S |2 ++
3 files changed, 35 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/include/asm/opal-api.h
b/arch/powerpc/include/asm/opal-api.h
index 0321a90..64508eb 100644
--- a/arch/powerpc/include/asm/opal-api.h
+++ b/arch/powerpc/include/asm/opal-api.h
@@ -153,7 +153,9 @@
#define OPAL_FLASH_READ 110
#define OPAL_FLASH_WRITE 111
#define OPAL_FLASH_ERASE 112
-#define OPAL_LAST 112
+#define OPAL_LEDS_GET_INDICATOR114
+#define OPAL_LEDS_SET_INDICATOR115
+#define OPAL_LAST 115
/* Device tree flags */
@@ -730,6 +732,31 @@ struct opal_i2c_request {
__be64 buffer_ra; /* Buffer real address */
};
+/* LED Mode */
+#define LED_MODE_LIGHT_PATH"lightpath"
+#define LED_MODE_GUIDING_LIGHT "guidinglight"
+
+/* LED type */
+#define LED_TYPE_IDENTIFY "identify"
+#define LED_TYPE_FAULT "fault"
+#define LED_TYPE_ATTENTION "attention"
+
+/* LED location */
+#define LED_LOC_ENCLOSURE "enclosure"
+#define LED_LOC_DESCENDENT "descendent"
These macros should also have POWERNV_LED prefix.
+enum OpalSlotLedType {
+ OPAL_SLOT_LED_TYPE_ID = 0, /* IDENTIFY LED */
+ OPAL_SLOT_LED_TYPE_FAULT = 1, /* FAULT LED */
+ OPAL_SLOT_LED_TYPE_ATTN = 2,/* System Attention LED */
+ OPAL_SLOT_LED_TYPE_MAX = 3
+};
+
+enum OpalSlotLedState {
+ OPAL_SLOT_LED_STATE_OFF = 0,/* LED is OFF */
+ OPAL_SLOT_LED_STATE_ON = 1 /* LED is ON */
+};
+
#endif /* __ASSEMBLY__ */
#endif /* __OPAL_API_H */
diff --git a/arch/powerpc/include/asm/opal.h b/arch/powerpc/include/asm/opal.h
index 042af1a..e06dc7e 100644
--- a/arch/powerpc/include/asm/opal.h
+++ b/arch/powerpc/include/asm/opal.h
@@ -193,6 +193,11 @@ int64_t opal_ipmi_recv(uint64_t interface, struct
opal_ipmi_msg *msg,
uint64_t *msg_len);
int64_t opal_i2c_request(uint64_t async_token, uint32_t bus_id,
struct opal_i2c_request *oreq);
+int64_t opal_leds_get_ind(char *loc_code, u64 *led_mask,
+ u64 *led_value, u64 *max_led_type);
+int64_t opal_leds_set_ind(uint64_t token, char *loc_code, const u64 led_mask,
+ const u64 led_value, u64 *max_led_type);
+
int64_t opal_flash_read(uint64_t id, uint64_t offset, uint64_t buf,
uint64_t size, uint64_t token);
diff --git a/arch/powerpc/platforms/powernv/opal-wrappers.S
b/arch/powerpc/platforms/powernv/opal-wrappers.S
index 4e74037..20d4da4 100644
--- a/arch/powerpc/platforms/powernv/opal-wrappers.S
+++ b/arch/powerpc/platforms/powernv/opal-wrappers.S
@@ -295,3 +295,5 @@ OPAL_CALL(opal_i2c_request,
OPAL_I2C_REQUEST);
OPAL_CALL(opal_flash_read,OPAL_FLASH_READ);
OPAL_CALL(opal_flash_write, OPAL_FLASH_WRITE);
OPAL_CALL(opal_flash_erase, OPAL_FLASH_ERASE);
+OPAL_CALL(opal_leds_get_ind, OPAL_LEDS_GET_INDICATOR);
+OPAL_CALL(opal_leds_set_ind, OPAL_LEDS_SET_INDICATOR);
--
Best Regards,
Jacek Anaszewski
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
On 04/20/2015 02:34 PM, Vasant Hegde wrote: On 04/20/2015 05:15 PM, Jacek Anaszewski wrote: Hi Vasant, I'd like to clarify some details regarding your explanation. On 04/15/2015 12:15 PM, Vasant Hegde wrote: [...] In Power Systems LEDs are overloaded (meaning same LED is used for identify and fault depending on their state --- blinking = identify and solid = fault). Hence here append LED type info. The label could be composed of segments and an ordinal number as labels have to be unique, e.g. attn_ident_0, attn_ident_1. The segments would have to be parsed by the driver to discover all the LED's available modes. nitpicking: identify is a verb and is not a proper name for the LED. Could you describe the purpose of this mode, so that we could come up with a better name? Each component (Field Replacement Unit) will have service indicator (LEDS) which can have below states : - OFF : no action - Identify: blinking state (user can use this state to identify particular component). In Power Systems world we call it as "identify" indicator.. Hence I retained same name here. How about just "ident" ? - fault : solid state (when component goes bad, LED goes to solid state) Note that our FW is capable of isolating some of the issues and it can turn on LEDs without OS interference. We have one more System level LED (System Attention Indicator).. This LED has two states: - OFF : Everything is fine - ON : Some component has issues and needs attention. We have three modes: - identify- blinking - fault- solid - attention indicator- solid How does LED operation differ for fault and attention modes? Does a LED have different intensity? Jacek, System Attention LED is special LED and its single LED available/system. where as identify and fault is applicable to all field replaceable units in the system.. So Typical server will have 1 System Attention LED N Identify/fault LED (N = Field Replaceable Unit). Apart from above two, we do have two more LEDs/Enclosure (external visible LEDs) - Enclosure Identify - Enclosure fault These LEDs reflects state of all Field Replaceable Units (FRU) inside this enclosure If any of FRU state is ON, this will become ON Also we can independently enable this LED!! But from kernel side implementation point of view, I just treat this as another LED.. as our platform code (OPAL firmware) takes care of roll up etc. Now our LED can operate in two mode (Depending on our service model, typically one/two socket servers are Light Path mode, whereas high end servers are Guiding Light Mode). 1. Guiding Light Only Identify indicator is support.. Fault is not supported System attention indicator is used to point there is some problem in system and need attention 2. Light Path mode Both identify and fault indicator is supported .. Fault is ON whenever some component is faulty System attention indicator is used to point that FW/OS is not able to isolate the problem and needs user to look into serviceable event (like syslog/ our agents like ESA which analyzes and reports events) Handling LED states : - Though physically single LED is overloaded for identify and fault, logically (FW/OS level) we treat them as separate LED. - We can enable both fault and identify simultaneously. - Hardware decides physical LED state (rule : identify has priority over fault). ex: Say location code 'X', Identify = ON, fault = ON , state of 'X' = identify (blinking) Identify = OFF, fault = ON, state of 'X' = fault (solid) Identify = OFF, fault = ON, state of 'X' = identify (blinking) Identify = OFF, fault = OFF , state of 'X' = OFF Since we have various above combinations, I thought its best to have separate class dev for each individual LEDs. That way we keep everything simple and let firmware handle all complexities. Hope this clarifies. I just posted v3 where I addressed your comments. https://lists.ozlabs.org/pipermail/linuxppc-dev/2015-April/127702.html Please let me know if you have any comments/suggestions. From what I can see from the driver code the LEDs are set with: opal_leds_set_ind(token, loc_code, led_mask, led_value, &max_led_type); and their state can be read with: opal_leds_get_ind(loc_code, &led_mask, &led_value, &max_led_type) From the kernel point of view these are very simple operations. All the logic you described should be handled by user space. If you need to be able to specify the LED mode you want to set/read, then additional 'mode' sysfs attribute should be added by the driver. There would have to be also additional sysfs attribute 'available_modes" provided. The ABI documentation should inform how the mode identifi
Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
Hi Vasant, I'd like to clarify some details regarding your explanation. On 04/15/2015 12:15 PM, Vasant Hegde wrote: [...] In Power Systems LEDs are overloaded (meaning same LED is used for identify and fault depending on their state --- blinking = identify and solid = fault). Hence here append LED type info. The label could be composed of segments and an ordinal number as labels have to be unique, e.g. attn_ident_0, attn_ident_1. The segments would have to be parsed by the driver to discover all the LED's available modes. nitpicking: identify is a verb and is not a proper name for the LED. Could you describe the purpose of this mode, so that we could come up with a better name? Each component (Field Replacement Unit) will have service indicator (LEDS) which can have below states : - OFF: no action - Identify: blinking state (user can use this state to identify particular component). In Power Systems world we call it as "identify" indicator.. Hence I retained same name here. How about just "ident" ? - fault : solid state (when component goes bad, LED goes to solid state) Note that our FW is capable of isolating some of the issues and it can turn on LEDs without OS interference. We have one more System level LED (System Attention Indicator).. This LED has two states: - OFF : Everything is fine - ON : Some component has issues and needs attention. We have three modes: - identify - blinking - fault - solid - attention indicator - solid How does LED operation differ for fault and attention modes? Does a LED have different intensity? I'd rather avoid creating separate LED class devices for each mode. For blinking we could use existing timer trigger. -- Best Regards, Jacek Anaszewski ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
Resending, as there were some problems with delivering this message. Original Message Subject: Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform Date: Thu, 16 Apr 2015 13:34:17 +0200 From: Jacek Anaszewski To: Vasant Hegde CC: linuxppc-dev@lists.ozlabs.org, linux-l...@vger.kernel.org, stew...@linux.vnet.ibm.com, m...@ellerman.id.au, coolo...@gmail.com, rpur...@rpsys.net, khand...@linux.vnet.ibm.com On 04/16/2015 12:26 PM, Vasant Hegde wrote: On 04/16/2015 02:21 PM, Jacek Anaszewski wrote: Hi Vasant, On 04/16/2015 08:52 AM, Vasant Hegde wrote: On 04/15/2015 06:42 PM, Jacek Anaszewski wrote: On 04/15/2015 12:15 PM, Vasant Hegde wrote: On 04/15/2015 02:12 PM, Jacek Anaszewski wrote: Hi Vasant, Hi Jacek, .../... I mean, we have to retain the state of LED across system reboot. Static variables are reinitialized on system reboot, aren't they? Sorry. I think comment was confusing.. As I understood, classdev_unregister call resets all LEDs state. However in our case, we don't want to change the LED state during system shutdown/reboot. Hence I have introduced state variable here. So during register call, I just disable LEDs so that any further call will just return. That way we retain LED state even after unloading module. Please let me know if there is any better way to achieve this. Since this is not a feature of the device, but rather a use case, then it cannot be hard coded in the driver this way. The solution I see is introducing a sysfs attribute that would determine if we want the LED to be turned off on unregistration or not. Hmmm. I thought having simple various is enough .. I don't know the usage of other LED drivers .. Just a thought .. How about enabling this feature in LED class itself ...Something like: - Add new attribute to generic LED class (say persistent)? - If drivers wants to retain LED state across reboot, then enable this option - During unregister call check for this attribute You can refer to the following driver to find out how to add the sysfs attribute: drivers/leds/leds-lm3533.c The attribute will also have to be documented, similarly to these: Documentation/ABI/testing/sysfs-class-led-driver-lm3533 Currently I don't have a good candidate for attribute name, so feel free to propose one. If you don't like generic attribute, then shall I introduce "persistent" attribute inside my driver. ? - By default this attribute is ON and if user wants he can disable this . - And I will have another variable (say op_support).. which I will disable in unload path.. .../... The label could be composed of segments and an ordinal number as labels have to be unique, e.g. attn_ident_0, attn_ident_1. The segments would have to be parsed by the driver to discover all the LED's available modes. nitpicking: identify is a verb and is not a proper name for the LED. Could you describe the purpose of this mode, so that we could come up with a better name? Each component (Field Replacement Unit) will have service indicator (LEDS) which can have below states : - OFF : no action - Identify: blinking state (user can use this state to identify particular component). In Power Systems world we call it as "identify" indicator.. Hence I retained same name here. How about just "ident" ? Sounds good. - fault : solid state (when component goes bad, LED goes to solid state) Note that our FW is capable of isolating some of the issues and it can turn on LEDs without OS interference. Does it mean that the LED can be controlled from hardware? If so, what would be software use cases then? The same question is related to the attn and indent states. - Identify LEDs: We have service processor interface to set/reset this LEDs.. Similar operation can be done from inband interface as well (via user space tools in Linux).. Later management layer can make use of this. - Fault / Attention : FW can SET these LEDs if its capable of isolating problem. Similarly host/user space can SET these LEDs if then can do fine grained problem isolation etc. Host/user space can RESET these LEDs. Hence we are adding host support to all the LEDs which can be modified by host. Hope this clarifies. Thanks for this explanation. I am changing my mind about these LEDs. Since they can be controlled from hardware without any system interaction, then turning them off on driver removal makes no sense. The LEDs could be turned on again even if the driver is unloaded. Since the driver doesn't perform any initialization of the LEDs, neither should it turn them off on removal. If I understand this correctly, then the solution with variable would do and no sysfs attribute would be required. Jacek, Thanks. Then I will retain current method. One question..What is the preferred way to name LED node in this case
Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
On 04/16/2015 12:26 PM, Vasant Hegde wrote:
On 04/16/2015 02:21 PM, Jacek Anaszewski wrote:
Hi Vasant,
On 04/16/2015 08:52 AM, Vasant Hegde wrote:
On 04/15/2015 06:42 PM, Jacek Anaszewski wrote:
On 04/15/2015 12:15 PM, Vasant Hegde wrote:
On 04/15/2015 02:12 PM, Jacek Anaszewski wrote:
Hi Vasant,
Hi Jacek,
.../...
I mean, we have to retain the state of LED across system reboot.
Static variables are reinitialized on system reboot, aren't they?
Sorry. I think comment was confusing..
As I understood, classdev_unregister call resets all LEDs state. However in our
case, we don't
want to change the LED state during system shutdown/reboot.
Hence I have introduced state variable here. So during register call, I just
disable LEDs so that any further call will just return. That way we retain LED
state even after unloading module.
Please let me know if there is any better way to achieve this.
Since this is not a feature of the device, but rather a use case, then
it cannot be hard coded in the driver this way. The solution I see is
introducing a sysfs attribute that would determine if we want the
LED to be turned off on unregistration or not.
Hmmm. I thought having simple various is enough ..
I don't know the usage of other LED drivers .. Just a thought .. How about
enabling this feature in LED class itself ...Something like:
- Add new attribute to generic LED class (say persistent)?
- If drivers wants to retain LED state across reboot, then enable this
option
- During unregister call check for this attribute
You can refer to the following driver to find out how to add the
sysfs attribute:
drivers/leds/leds-lm3533.c
The attribute will also have to be documented, similarly to these:
Documentation/ABI/testing/sysfs-class-led-driver-lm3533
Currently I don't have a good candidate for attribute
name, so feel free to propose one.
If you don't like generic attribute, then shall I introduce "persistent"
attribute inside my driver. ?
- By default this attribute is ON and if user wants he can disable this .
- And I will have another variable (say op_support).. which I will disable
in
unload path..
.../...
The label could be composed of segments and an ordinal number as
labels have to be unique, e.g. attn_ident_0, attn_ident_1.
The segments would have to be parsed by the driver to discover
all the LED's available modes.
nitpicking: identify is a verb and is not a proper name for the LED.
Could you describe the purpose of this mode, so that we could come
up with a better name?
Each component (Field Replacement Unit) will have service indicator (LEDS)
which
can have below states :
- OFF : no action
- Identify: blinking state (user can use this state to identify particular
component).
In Power Systems world we call it as "identify" indicator.. Hence I
retained same name here.
How about just "ident" ?
Sounds good.
- fault : solid state (when component goes bad, LED goes to solid state)
Note that our FW is capable of isolating some of the issues and it
can turn
on LEDs without OS
interference.
Does it mean that the LED can be controlled from hardware?
If so, what would be software use cases then? The same question is
related to the attn and indent states.
- Identify LEDs:
We have service processor interface to set/reset this LEDs.. Similar
operation
can be done from inband interface as well (via user space tools in Linux)..
Later management layer can make use of this.
- Fault / Attention :
FW can SET these LEDs if its capable of isolating problem.
Similarly host/user space can SET these LEDs if then can do fine grained
problem isolation etc.
Host/user space can RESET these LEDs.
Hence we are adding host support to all the LEDs which can be modified by host.
Hope this clarifies.
Thanks for this explanation.
I am changing my mind about these LEDs. Since they can be controlled
from hardware without any system interaction, then turning them off
on driver removal makes no sense. The LEDs could be turned on again even
if the driver is unloaded.
Since the driver doesn't perform any initialization of the LEDs,
neither should it turn them off on removal.
If I understand this correctly, then the solution with variable would
do and no sysfs attribute would be required.
Jacek,
Thanks. Then I will retain current method.
One question..What is the preferred way to name LED node in this case ?
:
OR
ident <- attribute under each node
fault
attention
If possible locations are eclosure/descendent as in the documentation
you gave a reference to, then the related identifiers could be:
enclosure: encl
descendent: desc or fru (how does fru expand btw?)
Child nodes could be defined as follows:
led0 {
label = "powernv0:encl:attn:ident:fault"
}
led1 {
Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
Hi Vasant, On 04/16/2015 08:52 AM, Vasant Hegde wrote: On 04/15/2015 06:42 PM, Jacek Anaszewski wrote: On 04/15/2015 12:15 PM, Vasant Hegde wrote: On 04/15/2015 02:12 PM, Jacek Anaszewski wrote: Hi Vasant, Hi Jacek, .../... I mean, we have to retain the state of LED across system reboot. Static variables are reinitialized on system reboot, aren't they? Sorry. I think comment was confusing.. As I understood, classdev_unregister call resets all LEDs state. However in our case, we don't want to change the LED state during system shutdown/reboot. Hence I have introduced state variable here. So during register call, I just disable LEDs so that any further call will just return. That way we retain LED state even after unloading module. Please let me know if there is any better way to achieve this. Since this is not a feature of the device, but rather a use case, then it cannot be hard coded in the driver this way. The solution I see is introducing a sysfs attribute that would determine if we want the LED to be turned off on unregistration or not. Hmmm. I thought having simple various is enough .. I don't know the usage of other LED drivers .. Just a thought .. How about enabling this feature in LED class itself ...Something like: - Add new attribute to generic LED class (say persistent)? - If drivers wants to retain LED state across reboot, then enable this option - During unregister call check for this attribute You can refer to the following driver to find out how to add the sysfs attribute: drivers/leds/leds-lm3533.c The attribute will also have to be documented, similarly to these: Documentation/ABI/testing/sysfs-class-led-driver-lm3533 Currently I don't have a good candidate for attribute name, so feel free to propose one. If you don't like generic attribute, then shall I introduce "persistent" attribute inside my driver. ? - By default this attribute is ON and if user wants he can disable this . - And I will have another variable (say op_support).. which I will disable in unload path.. .../... The label could be composed of segments and an ordinal number as labels have to be unique, e.g. attn_ident_0, attn_ident_1. The segments would have to be parsed by the driver to discover all the LED's available modes. nitpicking: identify is a verb and is not a proper name for the LED. Could you describe the purpose of this mode, so that we could come up with a better name? Each component (Field Replacement Unit) will have service indicator (LEDS) which can have below states : - OFF : no action - Identify: blinking state (user can use this state to identify particular component). In Power Systems world we call it as "identify" indicator.. Hence I retained same name here. How about just "ident" ? Sounds good. - fault : solid state (when component goes bad, LED goes to solid state) Note that our FW is capable of isolating some of the issues and it can turn on LEDs without OS interference. Does it mean that the LED can be controlled from hardware? If so, what would be software use cases then? The same question is related to the attn and indent states. - Identify LEDs: We have service processor interface to set/reset this LEDs.. Similar operation can be done from inband interface as well (via user space tools in Linux).. Later management layer can make use of this. - Fault / Attention : FW can SET these LEDs if its capable of isolating problem. Similarly host/user space can SET these LEDs if then can do fine grained problem isolation etc. Host/user space can RESET these LEDs. Hence we are adding host support to all the LEDs which can be modified by host. Hope this clarifies. Thanks for this explanation. I am changing my mind about these LEDs. Since they can be controlled from hardware without any system interaction, then turning them off on driver removal makes no sense. The LEDs could be turned on again even if the driver is unloaded. Since the driver doesn't perform any initialization of the LEDs, neither should it turn them off on removal. If I understand this correctly, then the solution with variable would do and no sysfs attribute would be required. -- Best Regards, Jacek Anaszewski ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform
OPAL_SLOT_LED_TYPE_FAULT);
+ }
+ return rc;
+}
+
+/* Platform driver remove */
+static int powernv_led_remove(struct platform_device *pdev)
+{
+ struct powernv_led *pled;
+ struct powernv_led_work *pwork;
+ unsigned long flags;
+
+ /* Disable LED operation */
+ led_disabled = true;
+
+ pr_info("Unregister all classdev structures\n");
+ list_for_each_entry(pled, &powernv_led_list, link)
+ led_classdev_unregister(&pled->cdev);
+
+ pr_info("Wait for all work tasks to finish\n");
+ list_for_each_entry(pwork, &powernv_led_work_list, link)
+ flush_work(&pwork->work);
+
+ /* Free nodes from the work list */
+ powernv_led_compact_work_list();
+
+ /* Free nodes from the classdev list */
+ spin_lock_irqsave(&powernv_led_spinlock, flags);
+ while (!list_empty(&powernv_led_list)) {
+ pled = list_first_entry(&powernv_led_list,
+ struct powernv_led, link);
+ list_del(&pled->link);
+ kfree(pled);
+ }
+ spin_unlock_irqrestore(&powernv_led_spinlock, flags);
+
+ /* Check for memory leaks */
+ if (!list_empty(&powernv_led_work_list))
+ pr_warn("Work list not empty, memory leak\n");
+
+ if (!list_empty(&powernv_led_cmd_list))
+ pr_warn("Request list not empty, memory leak\n");
+
+ if (!list_empty(&powernv_led_list))
+ pr_warn("Classdev list not empty, memory leak\n");
+
+ return 0;
+}
+
+/* Platform driver property match */
+static const struct of_device_id powernv_led_match[] = {
+ {
+ .compatible = "ibm,opal-v3-led",
+ },
+ {},
+};
+MODULE_DEVICE_TABLE(of, powernv_led_match);
+
+static struct platform_driver powernv_led_driver = {
+ .probe = powernv_led_probe,
+ .remove = powernv_led_remove,
+ .driver = {
+ .name = "powernv-led-driver",
+ .owner = THIS_MODULE,
+ .of_match_table = powernv_led_match,
Is somewhere DT documentation available for these leds?
+ },
+};
+
+module_platform_driver(powernv_led_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("PowerNV LED driver");
+MODULE_AUTHOR("Anshuman Khandual ");
--
To unsubscribe from this list: send the line "unsubscribe linux-leds" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Best Regards,
Jacek Anaszewski
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
