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

[Sebastian Gottschall]

Am 20.02.2018 um 17:52 schrieb Steve deRosier:
>
>> +static int ath10k_register_gpio_chip(struct ath10k *ar)
>> +{
>> +       struct ath10k_gpiocontrol *gpio;
>> +       gpio = kzalloc(sizeof(struct ath10k_gpiocontrol), GFP_KERNEL);
>> +       if (!gpio) {
>> +               return -ENOMEM;
>> +       }
>> +
>> +       snprintf(gpio->label, sizeof(gpio->label), "ath10k-%s",
>> +                wiphy_name(ar->hw->wiphy));
>> +#if LINUX_VERSION_CODE >= KERNEL_VERSION(4,5,0)
>> +       gpio->gchip.parent = ar->dev;
>> +#else
>> +       gpio->gchip.dev = ar->dev;
>> +#endif
>> +       gpio->gchip.base = -1;  /* determine base automatically */
> I may be wrong, but I think that version guards are unwelcome here.
> The patch is supposed to go and apply specifically to the upstream
> version, which is clearly >= KERNEL_VERSION(4,5,0). I think you can
> drop this, only use the 'gpio->gchip.parent = ar->dev;' and call it
> good. This fixup should be picked up by Backports.  I understand that
> you might need to keep it for your internal tree, but I don't think
> the maintainers want to see this.
the original patch comes from my sources which are supposed to work with 
compat-wireless für multiple kernels. but for sure i can change that :-)
>
>
>> +/* remove GPIO chip */
>> +static void ath10k_unregister_gpio_chip(struct ath10k *ar)
>> +{
>> +#ifdef CONFIG_GPIOLIB
>> +       struct ath10k_gpiocontrol *gpio = ar->gpio;
>> +       if (gpio) {
>> +               gpiochip_remove(&gpio->gchip);
>> +               kfree(gpio);
>> +       }
>> +#endif
>> +}
> Instead of entering/exiting the guards inside every function, I think
> the usually accepted way to handle this is to place a block in the
> that looks basically like:
>
> #ifdef CONFIG_GPIOLIB
> static void ath10k_unregister_gpio_chip(struct ath10k *ar) {
>         struct ath10k_gpiocontrol *gpio = ar->gpio;
>         if (gpio) {
>                 gpiochip_remove(&gpio->gchip);
>                 kfree(gpio);
>         }
>   }
>
> next function....
> next function...
>
> #else
> static void ath10k_unregister_gpio_chip(struct ath10k *ar) {
>   }
> next function....
> next function...
> #endif
>
> If they weren't static, you'd do it in the .h file, with the
> declaration of the full function there and the definition in the .c.
>
> I may be nit-picking and your style is perfectly acceptable to the
> other maintainers.
i'm not a fan of bloat code. and making funktions static which are just 
called one will allow the compiler to optimize the code better
than making it non static. that makes no sense here for sure. its just a 
question how you handle it.
>
>
>> +
>> +static void ath10k_unregister_led(struct ath10k *ar)
>> +{
>> +#if defined (CONFIG_GPIOLIB) && defined(CONFIG_LEDS_CLASS)
>> +       if (ar->gpio)
>> +               led_classdev_unregister(&ar->gpio->cdev);
>> +#endif
>> +}
>> +
>> +
>>   int ath10k_core_start(struct ath10k *ar, enum ath10k_firmware_mode mode,
>>                        const struct ath10k_fw_components *fw)
>>   {
>>          int status;
>>          u32 val;
>> -
>>          lockdep_assert_held(&ar->conf_mutex);
>>
> Whitespace change for no apparent reason.
count it as typo
>
>> @@ -2372,8 +2524,32 @@ int ath10k_core_start(struct ath10k *ar, enum ath10k_firmware_mode mode,
>>          if (status)
>>                  goto err_hif_stop;
>>
>> -       return 0;
>>
>> +#ifdef CONFIG_GPIOLIB
>> +       /* LED Code */
>> +       if (ar->hw_params.led_pin) { /* only attach if non zero since some chipsets are unsupported yet */
>> +               if (!ar->gpio_attached) {
>> +                       status = ath10k_register_gpio_chip(ar);
>> +                       if (status) {
>> +                               goto err_no_led;
>> +                       }
>> +#ifdef CONFIG_LEDS_CLASS
>> +                       ar->gpio_attached = 1;
>> +                       ar->gpio->wifi_led.active_low = 1;
>> +                       ar->gpio->wifi_led.gpio = ar->hw_params.led_pin;
>> +                       ar->gpio->wifi_led.name = ar->gpio->label;
>> +                       ar->gpio->wifi_led.default_state = LEDS_GPIO_DEFSTATE_KEEP;
>> +
>> +                       ath10k_add_led(ar, &ar->gpio->wifi_led);
>> +#endif
>> +               }
>> +               ath10k_wmi_gpio_config(ar, ar->hw_params.led_pin, 0, WMI_GPIO_PULL_NONE, WMI_GPIO_INTTYPE_DISABLE); /* configure to output */
>> +               ath10k_wmi_gpio_output(ar, ar->hw_params.led_pin, 1);
>> +       }
>> +err_no_led:;
>> +#endif
>> +
>> +       return 0;
>>   err_hif_stop:
>>          ath10k_hif_stop(ar);
>>   err_htt_rx_detach:
> The guards make it harder to read. In this chunk, every component
> should be defined acceptably even if the CONFIG flags aren't defined
> because they're all under your control. Make sure they are, and then
> you can drop the guards here. The only trick is to be sure that any
> accessed struct members that are protected by guards are abstracted
> and only accessed inside your guarded functions. Alternately, you
> could encapsulate it into a function ath10k_core_gpios_start() and
> just have the single call there. The additional benefit being that you
> don't need the err_no_led: jump.
true, but consider that required structures like gpio_chip (ar->gpio) 
and led_classdev arent defined in the kernel if these CONFIG options 
arent configured. so i may move it into a separate function ,but the 
guards will remain.
thats not my fault. thats a fault of the kernel api.

> The rest of this looks OK to me.
okay. i already changed some code, based on your requests but will wait 
for reply of you and for other comments until i send out a new version
>
> - Steve
>
> --
> Steve deRosier
> Cal-Sierra Consulting LLC
> https://www.cal-sierra.com/
>

-- 
Mit freundlichen Grüssen / Regards

Sebastian Gottschall / CTO

NewMedia-NET GmbH - DD-WRT
Firmensitz:  Stubenwaldallee 21a, 64625 Bensheim
Registergericht: Amtsgericht Darmstadt, HRB 25473
Geschäftsführer: Peter Steinhäuser, Christian Scheele
http://www.dd-wrt.com
email: s.gottschall at dd-wrt.com
Tel.: +496251-582650 / Fax: +496251-5826565