[PATCH v12] ath10k: add LED and GPIO controlling support for various chipsets

[Julian Calaby]

Hi Felix,

On Fri, Mar 9, 2018 at 2:46 AM, Felix Fietkau <nbd at nbd.name> 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ł Miłecki:
>>> >>>On 2 March 2018 at 10:22, Sebastian Gottschall <s.gottschall at dd-wrt.com> 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 early
>>> >>>>>>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 limitation
>>> >>>I just reviewed some mips arch patch adding support for more LEDs for
>>> >>>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 function
>>> >>>multiple times and register multiple structs with LEDs.
>>> >>>
>>> >>>Isn't all you need something like this?
>>> >>>
>>> >>>static const struct gpio_led ath10k_leds[] = {
>>> >>>         {
>>> >>>                 .name = "ath10k:color:function",
>>> >>>                 .active_low = 1,
>>> >>>                 .default_state = LEDS_GPIO_DEFSTATE_KEEP,
>>> >>>         }
>>> >>>};
>>> >>>
>>> >>>static struct gpio_led_platform_data bcm47xx_leds_pdata_extra = {
>>> >>>         leds = ath10k_leds;
>>> >>>         num_leds = ARRAY_SIZE(ath10k_leds);
>>> >>>};
>>> >>>
>>> >>>ath10k_leds.gpio = ar->hw_params.led_pin;
>>> >>>gpio_led_register_device(0, &ath10k_leds);
>>> >>the problem are other architectures which have already registered gpio_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 can 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,

-- 
Julian Calaby

Email: julian.calaby at gmail.com
Profile: http://www.google.com/profiles/julian.calaby/