Return-path: Received: from mail-wr0-f193.google.com ([209.85.128.193]:33545 "EHLO mail-wr0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751012AbeCJHo4 (ORCPT ); Sat, 10 Mar 2018 02:44:56 -0500 MIME-Version: 1.0 In-Reply-To: <1b1f6344-d588-d2f2-4b3d-a0a4043e801f@nbd.name> References: <20180226084406.2093-1-s.gottschall@dd-wrt.com> <82d8ac0c-b391-6099-4c7f-991cc35445aa@dd-wrt.com> <20180302090312.GA30267@amd> <890a4dcc-f549-6487-2ce5-3b62874cd266@dd-wrt.com> <20180308090216.GC17761@amd> <20180308140515.GA4889@amd> <1b1f6344-d588-d2f2-4b3d-a0a4043e801f@nbd.name> From: Julian Calaby Date: Sat, 10 Mar 2018 18:44:34 +1100 Message-ID: (sfid-20180310_084507_809845_5E1DD9BA) Subject: Re: [PATCH v12] ath10k: add LED and GPIO controlling support for various chipsets To: Felix Fietkau Cc: Pavel Machek , Sebastian Gottschall , =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= , "open list:LED SUBSYSTEM" , "linux-wireless@vger.kernel.org" , Kalle Valo , ath10k@lists.infradead.org, Sebastian Gottschall Content-Type: text/plain; charset="UTF-8" Sender: linux-wireless-owner@vger.kernel.org List-ID: Hi Felix, On Fri, Mar 9, 2018 at 2:46 AM, Felix Fietkau wrote: > On 2018-03-08 15:05, Pavel Machek wrote: >> On Thu 2018-03-08 13:33:29, Sebastian Gottschall wrote: >>> Am 08.03.2018 um 10:02 schrieb Pavel Machek: >>> >On Wed 2018-03-07 18:54:41, Sebastian Gottschall wrote: >>> >>Am 07.03.2018 um 17:22 schrieb Rafa=C5=82 Mi=C5=82ecki: >>> >>>On 2 March 2018 at 10:22, Sebastian Gottschall wrote: >>> >>>>>>leds-gpio is crap and limited. you can just register one platform= data at >>> >>>>>>kernel runtime since its identified by its object name "led-gpio"= but the >>> >>>>>>kernel forbidds to register 2 platform datas with the same name >>> >>>>>>consider the ar71xx devices with qca988x wifi chipsets. they all = have >>> >>>>>>already a led platform data registered >>> >>>>>>at boottime. a second can't be registered anymore so gpio_led is = useless >>> >>>>>>at >>> >>>>>>all for most developers on such platforms. its mainly used for ea= rly >>> >>>>>>kernel >>> >>>>>>platform data initialisation for system leds. >>> >>>>>If leds-gpio has limitations, please fix those, rather then >>> >>>>>introducing duplicated code. >>> >>>>there is no duplicated code introduced and there is no solution for= it. >>> >>>>consider that all wifi drivers with softled support >>> >>>>are going that way with registering a own led driver. see ath9k for >>> >>>>instance. gpio-led cannot be used for it and there is no way to >>> >>>>support multiple platform datas with the same name. its a kernel li= mitation >>> >>>I just reviewed some mips arch patch adding support for more LEDs fo= r >>> >>>selected devices: >>> >>>[PATCH] MIPS: BCM47XX: Add Luxul XAP1500/XWR1750 WiFi LEDs >>> >>>https://patchwork.linux-mips.org/patch/18681/ >>> >>> >>> >>>It seems to be simply adding another call to the >>> >>>gpio_led_register_device(). It seems to me you can call that functio= n >>> >>>multiple times and register multiple structs with LEDs. >>> >>> >>> >>>Isn't all you need something like this? >>> >>> >>> >>>static const struct gpio_led ath10k_leds[] =3D { >>> >>> { >>> >>> .name =3D "ath10k:color:function", >>> >>> .active_low =3D 1, >>> >>> .default_state =3D LEDS_GPIO_DEFSTATE_KEEP, >>> >>> } >>> >>>}; >>> >>> >>> >>>static struct gpio_led_platform_data bcm47xx_leds_pdata_extra =3D { >>> >>> leds =3D ath10k_leds; >>> >>> num_leds =3D ARRAY_SIZE(ath10k_leds); >>> >>>}; >>> >>> >>> >>>ath10k_leds.gpio =3D ar->hw_params.led_pin; >>> >>>gpio_led_register_device(0, &ath10k_leds); >>> >>the problem are other architectures which have already registered gpi= o_led >>> >>at system start like ar71xx >>> >>you cannot register a second one. so a independend led driver is a >>> >>requirement for direct control >>> >If the limitation indeed exists, please fix the limitation rather than >>> >working around it in each and every driver. >>> see ath9k. its exact the same implementation. >> >> Ok, so one more driver to fix. >> >>> in addition my variant does also work without gpiolib support. so it ca= n be >>> used even if the kernel is configured >>> without gpio support. >>> and not to forget, using a own led driver is more ligthweight from the = call >>> path for each led on / off event which is important for >>> low performance embedded devices >> >> We are not going to copy&paste code because such code works without >> libraries, and we are not going to copy&paste code because that uses >> less cache during calls. Sorry. >> >> NAK. Please fix your patch. Since this discussion seems to have taken a = rather weird turn, I've > taken a look at the specific code, and from my point of view the code > that sets up the LED (including callback) is so trivial that it's simply > not worth dealing with adding the leds-gpio driver to the mix. > It adds extra complexity and an extra dependency for no reason at all. > There's no extra functionality to be gained by using it. Stupid question: If the LED driver isn't using the GPIO subsystem (when enabled), what happens if the user uses the GPIO subsystem to fiddle with the pin the LED is connected to? Thanks, --=20 Julian Calaby Email: julian.calaby@gmail.com Profile: http://www.google.com/profiles/julian.calaby/