From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jacek Anaszewski Subject: Re: [PATCH v2 2/2] leds/powernv: Add driver for PowerNV platform Date: Wed, 15 Apr 2015 10:42:40 +0200 Message-ID: <552E2480.9060102@samsung.com> References: <20150320105921.14866.83209.stgit@localhost.localdomain> <20150320110328.14866.98225.stgit@localhost.localdomain> <552D303A.5080506@samsung.com> <552E049D.2030604@linux.vnet.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from mailout1.w1.samsung.com ([210.118.77.11]:38354 "EHLO mailout1.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752307AbbDOImp (ORCPT ); Wed, 15 Apr 2015 04:42:45 -0400 Received: from eucpsbgm2.samsung.com (unknown [203.254.199.245]) by mailout1.w1.samsung.com (Oracle Communications Messaging Server 7.0.5.31.0 64bit (built May 5 2014)) with ESMTP id <0NMU0061QB1YEQ50@mailout1.w1.samsung.com> for linux-leds@vger.kernel.org; Wed, 15 Apr 2015 09:46:46 +0100 (BST) In-reply-to: <552E049D.2030604@linux.vnet.ibm.com> Sender: linux-leds-owner@vger.kernel.org List-Id: linux-leds@vger.kernel.org To: Vasant Hegde Cc: linuxppc-dev@lists.ozlabs.org, linux-leds@vger.kernel.org, stewart@linux.vnet.ibm.com, mpe@ellerman.id.au, cooloney@gmail.com, rpurdie@rpsys.net, khandual@linux.vnet.ibm.com Hi Vasant, On 04/15/2015 08:26 AM, Vasant Hegde wrote: > On 04/14/2015 08:50 PM, Jacek Anaszewski wrote: >> Hi Vasant, > > Hi Jacek, > >>> >>> This patch implements LED driver for PowerNV platform using the existing >>> generic LED class framework. It registers 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. >>> >>> 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. >>> >>> 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. >> >> Wouldn't it be easier to implement synchronization on the OPAL side? > > We had thought about this.. But OPAL intern depends on service processor to > enable/disable indicator. So we can't make this as synchronous one. I've revised this one more time and I think that use of spin locks is an overkill in this case. It would suffice to define three work queues - one per led and one common mutex. powernv_led_set and powernv_led_get would have to be called under the mutex. Please refer to the existing LED class drivers of the LED controllers with many sub-LEDs to control: e.g.: drivers/leds/leds-lm355x.c > > .../... > >>> >>> diff --git a/arch/powerpc/platforms/powernv/opal.c >>> b/arch/powerpc/platforms/powernv/opal.c >>> index 142a08a..fbfd9c1 100644 >>> --- a/arch/powerpc/platforms/powernv/opal.c >>> +++ b/arch/powerpc/platforms/powernv/opal.c >>> @@ -746,7 +746,7 @@ static void __init opal_irq_init(struct device_node *dn) >>> >>> static int __init opal_init(void) >>> { >>> - struct device_node *np, *consoles; >>> + struct device_node *np, *consoles, *led; >>> int rc; >>> >>> opal_node = of_find_node_by_path("/ibm,opal"); >>> @@ -772,6 +772,13 @@ static int __init opal_init(void) >>> /* Create i2c platform devices */ >>> opal_i2c_create_devs(); >>> >>> + /* Create led platform devices */ >>> + led = of_find_node_by_path("/ibm,opal/led"); >>> + if (led) { >>> + of_platform_device_create(led, "opal_led", NULL); >>> + of_node_put(led); >>> + } >>> + >>> /* Find all OPAL interrupts and request them */ >>> opal_irq_init(opal_node); >>> >>> @@ -904,3 +911,6 @@ EXPORT_SYMBOL_GPL(opal_rtc_write); >>> EXPORT_SYMBOL_GPL(opal_tpo_read); >>> EXPORT_SYMBOL_GPL(opal_tpo_write); >>> EXPORT_SYMBOL_GPL(opal_i2c_request); >>> +/* Export these symbols for PowerNV LED class driver */ >>> +EXPORT_SYMBOL_GPL(opal_leds_get_ind); >>> +EXPORT_SYMBOL_GPL(opal_leds_set_ind); >> >> Please split the above part to the separate patch and put it in the >> series before this one. > > Sure. Will split it in next version. > >> >>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig >>> index 25b320d..a93223c 100644 >>> --- a/drivers/leds/Kconfig >>> +++ b/drivers/leds/Kconfig >>> @@ -508,6 +508,15 @@ 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 >> >> OF dependency is missing here. > > Agree. Will fix. > >> >>> + help >>> + This option enables support for the system LEDs present on >>> + PowerNV platforms. Say 'y' to enable this support in kernel. >>> + Say 'm' enable this support as module. >> >> Please change the last line to: >> >> To compile this driver as a module, choose M here: the module will >> be called leds-powernv. >> > > Sure. Will fix. > >> >>> + >>> 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 cbba921..604ffc9 100644 >>> --- a/drivers/leds/Makefile >>> +++ b/drivers/leds/Makefile >>> @@ -58,6 +58,7 @@ obj-$(CONFIG_LEDS_BLINKM) += leds-blinkm.o >>> obj-$(CONFIG_LEDS_SYSCON) += leds-syscon.o >>> obj-$(CONFIG_LEDS_VERSATILE) += leds-versatile.o >>> obj-$(CONFIG_LEDS_MENF21BMC) += leds-menf21bmc.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 0000000..0c9f958 >>> --- /dev/null >>> +++ b/drivers/leds/leds-powernv.c >>> @@ -0,0 +1,620 @@ >>> +/* >>> + * PowerNV LED Driver >>> + * >>> + * Copyright IBM Corp. 2015 >>> + * >>> + * Author: Anshuman Khandual >>> + * Author: Vasant Hegde >>> + * >>> + * 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. >>> + */ >>> + >>> +#define PREFIX "POWERNV_LED" >>> +#define pr_fmt(fmt) PREFIX ": " fmt >> >> Wouldn't you mind using dev_* prefixed logging? >> Unless you have a good reason not to do it. >> > > I don't see any specific reason to use pr_fmt. Will convert this to dev_*. > > >>> + >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >>> +#include >> >> Please keep alphabetical order. > > Sure. > >> >>> + >>> +#include >>> + >>> +#define LED_STR_ATTENTION ":ATTENTION" >>> +#define LED_STR_IDENT ":IDENTIFY" >>> +#define LED_STR_FAULT ":FAULT" >> >> Namespacing prefix is required here. LED is reserved for >> LED subsystem global macros. How about POWERNV_LED_* ? > > Yep... Its my fault.. Makes sense to have POWERNV_*. You could also skip STR segment and have e.g. POWERNV_LED_ATTENTION. >> >>> + >>> +/* >>> + * LED operation state >>> + * >>> + * led_classdev_unregister resets the brightness values. However >>> + * we want to retain the LED state across boot. >> >> What boot are you thinking of here? >> >>> Hence disable >>> + * LED operation before calling led_classdev_unregister. >>> + */ >> >> This comment is unclear for me, as this is static initialization, >> unrelated to to any function call. >> > > I mean, we have to retain the state of LED across system reboot. Static variables are reinitialized on system reboot, aren't they? >>> +static bool led_disabled = false; >> >> Static variables are initialized to 0 by default. > > Fixed. > > .../... > >>> +/* >>> + * powernv_led_compact_work_list >>> + * >>> + * This function goes through the entire list of scheduled powernv_led_work >>> + * nodes and removes the nodes which have already processed one set LED >>> + * state request from request list and has been marked for deletion. This is >>> + * essential for cleaning the list before adding new elements into it. This >>> + * also tracks the total number of pending tasks. Once it reaches the >>> + * threshold the function will throttle till all the scheduled tasks completes >>> + * execution during which the user space thread will block and will be >>> + * prevented from queuing up more LED state change requests. >>> + */ >> >> Did you test the driver with led-triggers? > > I have tested set/reset LEDs .. not triggers as our user space dont' use that. At this moment it doesn't, but it can use them in the future. Besides, I think that you wouldn't like to see someone testing your driver with led-triggers and having it crashed? :) Testing LED subsystem drivers with led-triggers is useful also because some triggers (e.g. timer) call brightness_set op from the interrupt context. It allows to detect some non-obvious issues. >>> + >>> + switch (led_type) { >>> + case OPAL_SLOT_LED_TYPE_ATTN: >>> + loc_code[strlen(loc_code) - strlen(LED_STR_ATTENTION)] = '\0'; >>> + break; >>> + case OPAL_SLOT_LED_TYPE_ID: >>> + loc_code[strlen(loc_code) - strlen(LED_STR_IDENT)] = '\0'; >>> + break; >>> + case OPAL_SLOT_LED_TYPE_FAULT: >>> + loc_code[strlen(loc_code) - strlen(LED_STR_FAULT)] = '\0'; >>> + break; >>> + default: /* Unknown LED type */ >>> + goto out_loc; >>> + } >> >> Above sequence is repeated in the function below - it could be wrapped >> with a new function. > > Agree. Will fix. > > .../... > >>> + >>> +/* >>> + * power_led_classdev >>> + * >>> + * This function registers classdev structure for any given type of LED on >>> + * a given child LED device node. >>> + */ >>> +static int power_led_classdev(struct platform_device *pdev, >>> + struct device_node *cled, u64 led_type) >>> +{ >>> + int rc; >>> + unsigned long flags; >>> + struct powernv_led *cpled; >>> + >>> + cpled = kzalloc(sizeof(struct powernv_led), GFP_KERNEL); >>> + if (!cpled) { >>> + pr_err("Memory allocation failed at %s\n", __func__); >>> + return -ENOMEM; >>> + } >>> + >>> + /* Create the name for classdev */ >>> + switch (led_type) { >>> + case OPAL_SLOT_LED_TYPE_ATTN: >>> + cpled->cdev.name = kasprintf(GFP_KERNEL, "%s%s", >>> + cled->name, LED_STR_ATTENTION); >> >> We have a 'label' DT property for naming LED Flash class devices. >> Please refer to Documentation/devicetree/bindings/leds/common.txt. >> > > 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? > > .../... > >>> + >>> +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? > > These are PowerNV platform specific properties. I don't think its > specified/documented in kernel side. > These are documented in OPAL (firmware) side. Every driver using DT bindings needs DT documentation. Moreover the one exists for powerenv platform: Documentation/devicetree/bindings/hwmon/ibmpowernv.txt Bindings for these LEDs should be added there probably. -- Best Regards, Jacek Anaszewski