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

2018-04-06 Thread Kalle Valo
(adding back lists)

Sebastian Gottschall  writes:

> Am 06.04.2018 um 10:25 schrieb Kalle Valo:
>> Sebastian Gottschall  writes:
>>
>>> Am 06.04.2018 um 10:09 schrieb Kalle Valo:
 (adding back the lists, please don't top post and trim your quotes)

 Sebastian Gottschall  writes:

> okay. ath10k-check is buggy and doesnt work. so it doesnt help much
> with code styles
>
> root@seg-desktop:/xfs/ath10k/ath.gpio# ./ath10k-check
> global: 'drivers/net/wireless/ath/ath10k/Kconfig' is not a source file.
> Traceback (most recent call last):
>     File "./ath10k-check", line 461, in 
>       main()
>     File "./ath10k-check", line 455, in main
>       ret = run_checkpatch(args)
>     File "./ath10k-check", line 275, in run_checkpatch
>       output = subprocess.check_output(cmd, shell=True)
>     File "/usr/lib/python2.7/subprocess.py", line 219, in check_output
>       raise CalledProcessError(retcode, cmd, output=output)
> subprocess.CalledProcessError: Command 'global -f
> drivers/net/wireless/ath/ath10k/Kconfig' returned non-zero exit status 1
 You are missing gtags, see 'ath10k-check --help' for how to install:

 Requirements (all available in $PATH):

 * gcc
 * sparse
 * checkpatch.pl
 * gtags (from package global)
>>> ahm i did read this and this was all installed. but ath10k-check
>>> refuses to work
>>>
>>> root@seg-desktop:/home/seg/DEV# gtags --version
>>> gtags (GNU GLOBAL) 6.5.7
>>> Copyright (c) 1996-2017 Tama Communications Corporation
>>> License GPLv3+: GNU GPL version 3 or later
>>> 
>>> This is free software; you are free to change and redistribute it.
>>> There is NO WARRANTY, to the extent permitted by law.
>> Damn, I guess something has changed in gtags as I use older version:
>>
>> gtags - GNU GLOBAL 5.7.1
>
> okay. i will try to use a older version and see what happens

You can also run checkpatch manually (sorry for the long line):

checkpatch.pl --strict -q --terse --no-summary --max-line-length=90 
--show-types --ignore 
MSLEEP,USLEEP_RANGE,PRINTK_WITHOUT_KERN_LEVEL,NETWORKING_BLOCK_COMMENT_STYLE,LINUX_VERSION_CODE,COMPLEX_MACRO,PREFER_DEV_LEVEL,PREFER_PR_LEVEL,COMPARISON_TO_NULL,BIT_MACRO,CONSTANT_COMPARISON,MACRO_WITH_FLOW_CONTROL,CONST_STRUCT,MACRO_ARG_REUSE,MACRO_ARG_PRECEDENCE
 foo.patch

I modified ath10k-check --help to print the full commandline, just
haven't pushed it out yet.

-- 
Kalle Valo


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

2018-04-06 Thread Kalle Valo
Sebastian Gottschall  writes:

> Am 06.04.2018 um 10:09 schrieb Kalle Valo:
>> (adding back the lists, please don't top post and trim your quotes)
>>
>> Sebastian Gottschall  writes:
>>
>>> okay. ath10k-check is buggy and doesnt work. so it doesnt help much
>>> with code styles
>>>
>>> root@seg-desktop:/xfs/ath10k/ath.gpio# ./ath10k-check
>>> global: 'drivers/net/wireless/ath/ath10k/Kconfig' is not a source file.
>>> Traceback (most recent call last):
>>>    File "./ath10k-check", line 461, in 
>>>      main()
>>>    File "./ath10k-check", line 455, in main
>>>      ret = run_checkpatch(args)
>>>    File "./ath10k-check", line 275, in run_checkpatch
>>>      output = subprocess.check_output(cmd, shell=True)
>>>    File "/usr/lib/python2.7/subprocess.py", line 219, in check_output
>>>      raise CalledProcessError(retcode, cmd, output=output)
>>> subprocess.CalledProcessError: Command 'global -f
>>> drivers/net/wireless/ath/ath10k/Kconfig' returned non-zero exit status 1
>>
>> You are missing gtags, see 'ath10k-check --help' for how to install:
>>
>> Requirements (all available in $PATH):
>>
>> * gcc
>> * sparse
>> * checkpatch.pl
>> * gtags (from package global)
>
> ahm i did read this and this was all installed. but ath10k-check
> refuses to work
>
> root@seg-desktop:/home/seg/DEV# gtags --version
> gtags (GNU GLOBAL) 6.5.7
> Copyright (c) 1996-2017 Tama Communications Corporation
> License GPLv3+: GNU GPL version 3 or later
> 
> This is free software; you are free to change and redistribute it.
> There is NO WARRANTY, to the extent permitted by law.

Damn, I guess something has changed in gtags as I use older version:

gtags - GNU GLOBAL 5.7.1

-- 
Kalle Valo


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

2018-04-06 Thread Sebastian Gottschall

Am 06.04.2018 um 10:09 schrieb Kalle Valo:

(adding back the lists, please don't top post and trim your quotes)

Sebastian Gottschall  writes:


okay. ath10k-check is buggy and doesnt work. so it doesnt help much
with code styles

root@seg-desktop:/xfs/ath10k/ath.gpio# ./ath10k-check
global: 'drivers/net/wireless/ath/ath10k/Kconfig' is not a source file.
Traceback (most recent call last):
   File "./ath10k-check", line 461, in 
     main()
   File "./ath10k-check", line 455, in main
     ret = run_checkpatch(args)
   File "./ath10k-check", line 275, in run_checkpatch
     output = subprocess.check_output(cmd, shell=True)
   File "/usr/lib/python2.7/subprocess.py", line 219, in check_output
     raise CalledProcessError(retcode, cmd, output=output)
subprocess.CalledProcessError: Command 'global -f
drivers/net/wireless/ath/ath10k/Kconfig' returned non-zero exit status 1

You are missing gtags, see 'ath10k-check --help' for how to install:

Requirements (all available in $PATH):

* gcc
* sparse
* checkpatch.pl
* gtags (from package global)
ahm i did read this and this was all installed. but ath10k-check refuses 
to work


root@seg-desktop:/home/seg/DEV# gtags --version
gtags (GNU GLOBAL) 6.5.7
Copyright (c) 1996-2017 Tama Communications Corporation
License GPLv3+: GNU GPL version 3 or later 


This is free software; you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law.





--
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.gottsch...@dd-wrt.com
Tel.: +496251-582650 / Fax: +496251-5826565



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

2018-04-06 Thread Sebastian Gottschall

Am 06.04.2018 um 10:07 schrieb Kalle Valo:

Sebastian Gottschall  writes:


i have some comments about your check warnings.
some of them are bogus. for instance they advise to use "unsigned int"
instead of "unsigned". this might be better, but
the original kernel header uses "unsigned" as api definition. so i
decided to use the same declarations here.

Sure, but using "unsigned int" won't create any harm either.


for the rest like whitespaces i will make a new version and remove them

You missed my comment in my earlier mail that I'll send the next
version. I have already fixed all the warnings so let me submit it. I'll
do also some other changes.


oh okay. thank you. then forget all the rest. i will take care of these 
issues with the next patch i submit






--
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.gottsch...@dd-wrt.com
Tel.: +496251-582650 / Fax: +496251-5826565



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

2018-04-06 Thread Kalle Valo
(adding back the lists, please don't top post and trim your quotes)

Sebastian Gottschall  writes:

> okay. ath10k-check is buggy and doesnt work. so it doesnt help much
> with code styles
>
> root@seg-desktop:/xfs/ath10k/ath.gpio# ./ath10k-check
> global: 'drivers/net/wireless/ath/ath10k/Kconfig' is not a source file.
> Traceback (most recent call last):
>   File "./ath10k-check", line 461, in 
>     main()
>   File "./ath10k-check", line 455, in main
>     ret = run_checkpatch(args)
>   File "./ath10k-check", line 275, in run_checkpatch
>     output = subprocess.check_output(cmd, shell=True)
>   File "/usr/lib/python2.7/subprocess.py", line 219, in check_output
>     raise CalledProcessError(retcode, cmd, output=output)
> subprocess.CalledProcessError: Command 'global -f 
> drivers/net/wireless/ath/ath10k/Kconfig' returned non-zero exit status 1

You are missing gtags, see 'ath10k-check --help' for how to install:

Requirements (all available in $PATH):

* gcc
* sparse
* checkpatch.pl
* gtags (from package global)

-- 
Kalle Valo


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

2018-04-06 Thread Kalle Valo
Sebastian Gottschall  writes:

> i have some comments about your check warnings.
> some of them are bogus. for instance they advise to use "unsigned int"
> instead of "unsigned". this might be better, but
> the original kernel header uses "unsigned" as api definition. so i
> decided to use the same declarations here.

Sure, but using "unsigned int" won't create any harm either.

> for the rest like whitespaces i will make a new version and remove them

You missed my comment in my earlier mail that I'll send the next
version. I have already fixed all the warnings so let me submit it. I'll
do also some other changes.

-- 
Kalle Valo


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

2018-04-06 Thread Kalle Valo
Sebastian Gottschall  writes:

> Am 05.04.2018 um 16:44 schrieb Kalle Valo:
>> s.gottsch...@dd-wrt.com writes:
>>
>>> Adds LED and GPIO Control support for 988x, 9887, 9888, 99x0, 9984
>>> based chipsets with on chipset connected led's using WMI Firmware API.
>>> The LED device will get available named as "ath10k-phyX" at sysfs and
>>> can be controlled with various triggers. adds also debugfs interface
>>> for gpio control.
>>>
>>> Signed-off-by: Sebastian Gottschall 
>> [...]
>>
>>> @@ -1034,7 +1068,7 @@ ath10k_wmi_pdev_get_temperature(struct ath10k *ar)
>>>   if (IS_ERR(skb))
>>>   return PTR_ERR(skb);
>>>   -    return ath10k_wmi_cmd_send(ar, skb,
>>> +    return ath10k_wmi_cmd_send_nowait(ar, skb,
>>> ar->wmi.cmd->pdev_get_temperature_cmdid);
>>>   }
>> This looks odd, I don't think it belongs to this patch.
>
> thats true. but due the nature of this function i found it better to
> use nowait here. better if i split it up?

Yes, this should be done in a separate patch with a proper commit log
explaining why it's needed.

>> Also you made a some sort of record, your patch had 181 checkpatch
>> warnings! Do you use Word as your editor or what? But please do check
>> your editor settings and read the coding style documents.
>
> no? i use midnight commander for all of my code since more than 20 years
> and its the first time that i see such warnings. is there any special
> coding rule for ath10k which differs from the kernel rules?

You got even the indentation wrong in multiple functions and indentation
rules have been the same as long as I remember. And checkpatch has been
around a long time already, that should not be new to anyone submitting
patches.

> and where is ath10k-check located?

Check the link I provided:

>> https://wireless.wiki.kernel.org/en/users/drivers/ath10k/codingstyle

-- 
Kalle Valo


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

2018-04-05 Thread Sebastian Gottschall

i have some comments about your check warnings.
some of them are bogus. for instance they advise to use "unsigned int" 
instead of "unsigned". this might be better, but
the original kernel header uses "unsigned" as api definition. so i 
decided to use the same declarations here.

for the rest like whitespaces i will make a new version and remove them

Am 05.04.2018 um 20:01 schrieb Sebastian Gottschall:

Am 05.04.2018 um 16:44 schrieb Kalle Valo:

s.gottsch...@dd-wrt.com writes:


Adds LED and GPIO Control support for 988x, 9887, 9888, 99x0, 9984
based chipsets with on chipset connected led's using WMI Firmware API.
The LED device will get available named as "ath10k-phyX" at sysfs and
can be controlled with various triggers. adds also debugfs interface
for gpio control.

Signed-off-by: Sebastian Gottschall 

[...]

@@ -1034,7 +1068,7 @@ ath10k_wmi_pdev_get_temperature(struct ath10k 
*ar)

  if (IS_ERR(skb))
  return PTR_ERR(skb);
  -    return ath10k_wmi_cmd_send(ar, skb,
+    return ath10k_wmi_cmd_send_nowait(ar, skb,
ar->wmi.cmd->pdev_get_temperature_cmdid);
  }

This looks odd, I don't think it belongs to this patch.
thats true. but due the nature of this function i found it better to 
use nowait here. better if i split it up?


Also you made a some sort of record, your patch had 181 checkpatch
warnings! Do you use Word as your editor or what? But please do check
your editor settings and read the coding style documents.

no? i use midnight commander for all of my code since more than 20 years
and its the first time that i see such warnings. is there any special 
coding rule for ath10k which differs from the kernel rules?

and where is ath10k-check located?


https://wireless.wiki.kernel.org/en/users/drivers/ath10k/codingstyle

I'll send v13.

Here are the warnings from ath10k-check:

drivers/net/wireless/ath/ath10k/gpio.c:19: Please don't use multiple 
blank lines
drivers/net/wireless/ath/ath10k/gpio.c:30: Please don't use multiple 
blank lines
drivers/net/wireless/ath/ath10k/gpio.c:36: Prefer 'unsigned int' to 
bare use of 'unsigned'

drivers/net/wireless/ath/ath10k/gpio.c:38: line over 90 characters
drivers/net/wireless/ath/ath10k/gpio.c:39: line over 90 characters
drivers/net/wireless/ath/ath10k/gpio.c:39: Missing a blank line after 
declarations
drivers/net/wireless/ath/ath10k/gpio.c:45: Prefer 'unsigned int' to 
bare use of 'unsigned'
drivers/net/wireless/ath/ath10k/gpio.c:46: Alignment should match 
open parenthesis

drivers/net/wireless/ath/ath10k/gpio.c:48: line over 90 characters
drivers/net/wireless/ath/ath10k/gpio.c:50: line over 90 characters
drivers/net/wireless/ath/ath10k/gpio.c:58: Prefer 'unsigned int' to 
bare use of 'unsigned'

drivers/net/wireless/ath/ath10k/gpio.c:60: line over 90 characters
drivers/net/wireless/ath/ath10k/gpio.c:66: Prefer 'unsigned int' to 
bare use of 'unsigned'

drivers/net/wireless/ath/ath10k/gpio.c:68: line over 90 characters
drivers/net/wireless/ath/ath10k/gpio.c:74: Prefer 'unsigned int' to 
bare use of 'unsigned'
drivers/net/wireless/ath/ath10k/gpio.c:75: Alignment should match 
open parenthesis

drivers/net/wireless/ath/ath10k/gpio.c:77: line over 90 characters
drivers/net/wireless/ath/ath10k/gpio.c:86: trailing whitespace
drivers/net/wireless/ath/ath10k/gpio.c:86: please, no spaces at the 
start of a line
drivers/net/wireless/ath/ath10k/gpio.c:86: Blank lines aren't 
necessary after an open brace '{'
drivers/net/wireless/ath/ath10k/gpio.c:88: Missing a blank line after 
declarations
drivers/net/wireless/ath/ath10k/gpio.c:88: braces {} are not 
necessary for single statement blocks

drivers/net/wireless/ath/ath10k/gpio.c:113: trailing whitespace
drivers/net/wireless/ath/ath10k/gpio.c:114: Missing a blank line 
after declarations
drivers/net/wireless/ath/ath10k/gpio.c:114: braces {} are not 
necessary for single statement blocks

drivers/net/wireless/ath/ath10k/gpio.c:121: line over 90 characters
drivers/net/wireless/ath/ath10k/gpio.c:130: Please don't use multiple 
blank lines
drivers/net/wireless/ath/ath10k/gpio.c:132: Alignment should match 
open parenthesis

drivers/net/wireless/ath/ath10k/gpio.c:134: line over 90 characters
drivers/net/wireless/ath/ath10k/gpio.c:136: Missing a blank line 
after declarations

drivers/net/wireless/ath/ath10k/gpio.c:145: trailing whitespace
drivers/net/wireless/ath/ath10k/gpio.c:146: Missing a blank line 
after declarations

drivers/net/wireless/ath/ath10k/gpio.c:159: trailing whitespace
drivers/net/wireless/ath/ath10k/gpio.c:160: Missing a blank line 
after declarations

drivers/net/wireless/ath/ath10k/gpio.c:166: trailing whitespace
drivers/net/wireless/ath/ath10k/gpio.c:169: trailing whitespace
drivers/net/wireless/ath/ath10k/gpio.c:170: line over 90 characters
drivers/net/wireless/ath/ath10k/gpio.c:178: Missing a blank line 
after declarations
drivers/net/wireless/ath/ath10k/gpio.c:181: Prefer 
kzalloc(sizeof(*gpio)...) over kzalloc(sizeof(struct 
ath10k_gpiocontrol)...)

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

2018-04-05 Thread Sebastian Gottschall

Am 05.04.2018 um 16:44 schrieb Kalle Valo:

s.gottsch...@dd-wrt.com writes:


Adds LED and GPIO Control support for 988x, 9887, 9888, 99x0, 9984
based chipsets with on chipset connected led's using WMI Firmware API.
The LED device will get available named as "ath10k-phyX" at sysfs and
can be controlled with various triggers. adds also debugfs interface
for gpio control.

Signed-off-by: Sebastian Gottschall 

[...]


@@ -1034,7 +1068,7 @@ ath10k_wmi_pdev_get_temperature(struct ath10k *ar)
  if (IS_ERR(skb))
  return PTR_ERR(skb);
  -    return ath10k_wmi_cmd_send(ar, skb,
+    return ath10k_wmi_cmd_send_nowait(ar, skb,
ar->wmi.cmd->pdev_get_temperature_cmdid);
  }

This looks odd, I don't think it belongs to this patch.
thats true. but due the nature of this function i found it better to use 
nowait here. better if i split it up?


Also you made a some sort of record, your patch had 181 checkpatch
warnings! Do you use Word as your editor or what? But please do check
your editor settings and read the coding style documents.

no? i use midnight commander for all of my code since more than 20 years
and its the first time that i see such warnings. is there any special 
coding rule for ath10k which differs from the kernel rules?

and where is ath10k-check located?


https://wireless.wiki.kernel.org/en/users/drivers/ath10k/codingstyle

I'll send v13.

Here are the warnings from ath10k-check:

drivers/net/wireless/ath/ath10k/gpio.c:19: Please don't use multiple 
blank lines
drivers/net/wireless/ath/ath10k/gpio.c:30: Please don't use multiple 
blank lines
drivers/net/wireless/ath/ath10k/gpio.c:36: Prefer 'unsigned int' to 
bare use of 'unsigned'

drivers/net/wireless/ath/ath10k/gpio.c:38: line over 90 characters
drivers/net/wireless/ath/ath10k/gpio.c:39: line over 90 characters
drivers/net/wireless/ath/ath10k/gpio.c:39: Missing a blank line after 
declarations
drivers/net/wireless/ath/ath10k/gpio.c:45: Prefer 'unsigned int' to 
bare use of 'unsigned'
drivers/net/wireless/ath/ath10k/gpio.c:46: Alignment should match open 
parenthesis

drivers/net/wireless/ath/ath10k/gpio.c:48: line over 90 characters
drivers/net/wireless/ath/ath10k/gpio.c:50: line over 90 characters
drivers/net/wireless/ath/ath10k/gpio.c:58: Prefer 'unsigned int' to 
bare use of 'unsigned'

drivers/net/wireless/ath/ath10k/gpio.c:60: line over 90 characters
drivers/net/wireless/ath/ath10k/gpio.c:66: Prefer 'unsigned int' to 
bare use of 'unsigned'

drivers/net/wireless/ath/ath10k/gpio.c:68: line over 90 characters
drivers/net/wireless/ath/ath10k/gpio.c:74: Prefer 'unsigned int' to 
bare use of 'unsigned'
drivers/net/wireless/ath/ath10k/gpio.c:75: Alignment should match open 
parenthesis

drivers/net/wireless/ath/ath10k/gpio.c:77: line over 90 characters
drivers/net/wireless/ath/ath10k/gpio.c:86: trailing whitespace
drivers/net/wireless/ath/ath10k/gpio.c:86: please, no spaces at the 
start of a line
drivers/net/wireless/ath/ath10k/gpio.c:86: Blank lines aren't 
necessary after an open brace '{'
drivers/net/wireless/ath/ath10k/gpio.c:88: Missing a blank line after 
declarations
drivers/net/wireless/ath/ath10k/gpio.c:88: braces {} are not necessary 
for single statement blocks

drivers/net/wireless/ath/ath10k/gpio.c:113: trailing whitespace
drivers/net/wireless/ath/ath10k/gpio.c:114: Missing a blank line after 
declarations
drivers/net/wireless/ath/ath10k/gpio.c:114: braces {} are not 
necessary for single statement blocks

drivers/net/wireless/ath/ath10k/gpio.c:121: line over 90 characters
drivers/net/wireless/ath/ath10k/gpio.c:130: Please don't use multiple 
blank lines
drivers/net/wireless/ath/ath10k/gpio.c:132: Alignment should match 
open parenthesis

drivers/net/wireless/ath/ath10k/gpio.c:134: line over 90 characters
drivers/net/wireless/ath/ath10k/gpio.c:136: Missing a blank line after 
declarations

drivers/net/wireless/ath/ath10k/gpio.c:145: trailing whitespace
drivers/net/wireless/ath/ath10k/gpio.c:146: Missing a blank line after 
declarations

drivers/net/wireless/ath/ath10k/gpio.c:159: trailing whitespace
drivers/net/wireless/ath/ath10k/gpio.c:160: Missing a blank line after 
declarations

drivers/net/wireless/ath/ath10k/gpio.c:166: trailing whitespace
drivers/net/wireless/ath/ath10k/gpio.c:169: trailing whitespace
drivers/net/wireless/ath/ath10k/gpio.c:170: line over 90 characters
drivers/net/wireless/ath/ath10k/gpio.c:178: Missing a blank line after 
declarations
drivers/net/wireless/ath/ath10k/gpio.c:181: Prefer 
kzalloc(sizeof(*gpio)...) over kzalloc(sizeof(struct 
ath10k_gpiocontrol)...)
drivers/net/wireless/ath/ath10k/gpio.c:182: braces {} are not 
necessary for single statement blocks

drivers/net/wireless/ath/ath10k/gpio.c:192: trailing whitespace
drivers/net/wireless/ath/ath10k/gpio.c:194: line over 90 characters
drivers/net/wireless/ath/ath10k/gpio.c:196: trailing whitespace
drivers/net/wireless/ath/ath10k/core.h:865: trailing whitespace
drivers/net/wireless/ath/ath10k/core.h:865: line over 90 

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

2018-04-05 Thread Kalle Valo
s.gottsch...@dd-wrt.com writes:

> Adds LED and GPIO Control support for 988x, 9887, 9888, 99x0, 9984
> based chipsets with on chipset connected led's using WMI Firmware API.
> The LED device will get available named as "ath10k-phyX" at sysfs and
> can be controlled with various triggers. adds also debugfs interface
> for gpio control.
>
> Signed-off-by: Sebastian Gottschall 

[...]

> @@ -1034,7 +1068,7 @@ ath10k_wmi_pdev_get_temperature(struct ath10k *ar)
>   if (IS_ERR(skb))
>   return PTR_ERR(skb);
>  
> - return ath10k_wmi_cmd_send(ar, skb,
> + return ath10k_wmi_cmd_send_nowait(ar, skb,
>  ar->wmi.cmd->pdev_get_temperature_cmdid);
>  }

This looks odd, I don't think it belongs to this patch.

Also you made a some sort of record, your patch had 181 checkpatch
warnings! Do you use Word as your editor or what? :) But please do check
your editor settings and read the coding style documents.

https://wireless.wiki.kernel.org/en/users/drivers/ath10k/codingstyle

I'll send v13.

Here are the warnings from ath10k-check:

drivers/net/wireless/ath/ath10k/gpio.c:19: Please don't use multiple blank lines
drivers/net/wireless/ath/ath10k/gpio.c:30: Please don't use multiple blank lines
drivers/net/wireless/ath/ath10k/gpio.c:36: Prefer 'unsigned int' to bare use of 
'unsigned'
drivers/net/wireless/ath/ath10k/gpio.c:38: line over 90 characters
drivers/net/wireless/ath/ath10k/gpio.c:39: line over 90 characters
drivers/net/wireless/ath/ath10k/gpio.c:39: Missing a blank line after 
declarations
drivers/net/wireless/ath/ath10k/gpio.c:45: Prefer 'unsigned int' to bare use of 
'unsigned'
drivers/net/wireless/ath/ath10k/gpio.c:46: Alignment should match open 
parenthesis
drivers/net/wireless/ath/ath10k/gpio.c:48: line over 90 characters
drivers/net/wireless/ath/ath10k/gpio.c:50: line over 90 characters
drivers/net/wireless/ath/ath10k/gpio.c:58: Prefer 'unsigned int' to bare use of 
'unsigned'
drivers/net/wireless/ath/ath10k/gpio.c:60: line over 90 characters
drivers/net/wireless/ath/ath10k/gpio.c:66: Prefer 'unsigned int' to bare use of 
'unsigned'
drivers/net/wireless/ath/ath10k/gpio.c:68: line over 90 characters
drivers/net/wireless/ath/ath10k/gpio.c:74: Prefer 'unsigned int' to bare use of 
'unsigned'
drivers/net/wireless/ath/ath10k/gpio.c:75: Alignment should match open 
parenthesis
drivers/net/wireless/ath/ath10k/gpio.c:77: line over 90 characters
drivers/net/wireless/ath/ath10k/gpio.c:86: trailing whitespace
drivers/net/wireless/ath/ath10k/gpio.c:86: please, no spaces at the start of a 
line
drivers/net/wireless/ath/ath10k/gpio.c:86: Blank lines aren't necessary after 
an open brace '{'
drivers/net/wireless/ath/ath10k/gpio.c:88: Missing a blank line after 
declarations
drivers/net/wireless/ath/ath10k/gpio.c:88: braces {} are not necessary for 
single statement blocks
drivers/net/wireless/ath/ath10k/gpio.c:113: trailing whitespace
drivers/net/wireless/ath/ath10k/gpio.c:114: Missing a blank line after 
declarations
drivers/net/wireless/ath/ath10k/gpio.c:114: braces {} are not necessary for 
single statement blocks
drivers/net/wireless/ath/ath10k/gpio.c:121: line over 90 characters
drivers/net/wireless/ath/ath10k/gpio.c:130: Please don't use multiple blank 
lines
drivers/net/wireless/ath/ath10k/gpio.c:132: Alignment should match open 
parenthesis
drivers/net/wireless/ath/ath10k/gpio.c:134: line over 90 characters
drivers/net/wireless/ath/ath10k/gpio.c:136: Missing a blank line after 
declarations
drivers/net/wireless/ath/ath10k/gpio.c:145: trailing whitespace
drivers/net/wireless/ath/ath10k/gpio.c:146: Missing a blank line after 
declarations
drivers/net/wireless/ath/ath10k/gpio.c:159: trailing whitespace
drivers/net/wireless/ath/ath10k/gpio.c:160: Missing a blank line after 
declarations
drivers/net/wireless/ath/ath10k/gpio.c:166: trailing whitespace
drivers/net/wireless/ath/ath10k/gpio.c:169: trailing whitespace
drivers/net/wireless/ath/ath10k/gpio.c:170: line over 90 characters
drivers/net/wireless/ath/ath10k/gpio.c:178: Missing a blank line after 
declarations
drivers/net/wireless/ath/ath10k/gpio.c:181: Prefer kzalloc(sizeof(*gpio)...) 
over kzalloc(sizeof(struct ath10k_gpiocontrol)...)
drivers/net/wireless/ath/ath10k/gpio.c:182: braces {} are not necessary for 
single statement blocks
drivers/net/wireless/ath/ath10k/gpio.c:192: trailing whitespace
drivers/net/wireless/ath/ath10k/gpio.c:194: line over 90 characters
drivers/net/wireless/ath/ath10k/gpio.c:196: trailing whitespace
drivers/net/wireless/ath/ath10k/core.h:865: trailing whitespace
drivers/net/wireless/ath/ath10k/core.h:865: line over 90 characters
drivers/net/wireless/ath/ath10k/core.h:871: trailing whitespace
drivers/net/wireless/ath/ath10k/core.h:890: Blank lines aren't necessary after 
an open brace '{'
drivers/net/wireless/ath/ath10k/core.h:891: Blank lines aren't necessary before 
a close brace '}'
drivers/net/wireless/ath/ath10k/core.h:892: Please use a blank line after 

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

2018-03-12 Thread Sebastian Gottschall

Am 12.03.2018 um 08:53 schrieb Mathias Kresin:

10.03.2018 08:56, Sebastian Gottschall:



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?


in our case here it can coexist and will not have a negative effect 
since the led will still run. (except if you reconfigure the gpio to 
input)

for sure it makes no sense.


Well, it will have negative effects as I noticed in OpenWrt. If the 
same GPIO is controlled by the internal LED function it can't be 
really used via the GPIO controller as there will be "bogus" changes 
of the GPIO pin state.
yes, it will have bogus changes, but it will not raise any other issues 
than the bogus changes your forcefully triggered. why disallowing 
something, if there is no reason for.

but i give you a example for a use.
if the wifi interface is down or not used, you can still take control 
about the led. so set the led to any state you want, no matter if the 
trigger is still installed


but however i can block the gpio for beeing reused if the led driver 
took it. this has been made in ath9k for instance.


Most likely I'm not aware of the upstream code you are talking about. 
But OpenWrt has a custom patch which registers the ath9k pins as GPIO 
controller.
thats the code i'm talking about, but as far as i know this code is 
upstream.


As soon as there are pins, manufactures will use them and the 
assumption about a default led connected to a specific pin will fail.
nope. manufactures are mainly using the qca propertiery driver and this 
qca driver uses fixed hardcoded led pins.
1 for qca988x and 17 for everything else. the led pins i assigned are 
following the qca specification for this chipsets and not to forget

i tested it on many devices


I've seen (home)routers routers using the "default" ath9k LED pin as 
button or for LEDs with a different purpose than "wireless". I never 
was able to find any kind of information which explains why exactly 
this specific pin is used for the ath9k-led. But that's another story.
i know. but thats not the case for ath10k so far and i'm following exact 
the same way qca is doing by themself.
i just know 2 cases. vendors to use the led pin of the chipset or they 
dont and use standard system leds with any assignment

and in all cases with no exception the default led pin was used.


My solution for OpenWrt was a patch which prevents the creation of the 
ath9k-led if the "default" pin is used as GPIO.
i have seen that code, but decided no to follow it since any developer 
is responsible for configuring whatever he wants. if it doesnt lead to a 
crash i see no reason for preventing it.

but thats my philosophy.


If mach files are used, the ath9k led isn't created (ath9k led pin is 
set to -1) in case the platform data have a button or led using the 
same pin. If the device tree is used, the ath9k led isn't created 
(ath9k led pin is set to -1) if the devicetree node for the ath9k 
device has the gpio-controller property. I'm not really proud of the 
code and it can be most likely done better, but it fixes the issue 
outlined above.
sounds very inflexible or better "intransparent". from the black box 
view its hard to understand how its handled without reading the code. 
its has no clean logic.
and your issue above is no issue. if a dev wants to make stupid things, 
let him do. he learns from his misstakes and he does not harm the 
hardware nor the stability.
there is no reason for extra hyper security barriers to guide someone 
how todo it. but here are samples to reuse a gpio from 2 points as i 
explained at the beginning.
i could imagine todo a manual error blinking or something if a interface 
is down. not sure, but other people might be more creative here.


I've done it this way since the use of the GPIO controller should 
always override any kind of default LEDs.


In the end ath[0|10]k-led is only required for hardware configurations 
where the wireless isn't fixed like with miniPCIe, USB ... wireless 
cards. If the configuration is fixed - like it is for most of the 
(home)routers - the GPIO controller can be registered via the 
devicetree and gpio-leds can be used. Something similar can be most 
likely done via mach files (I barely touch mach files).

ath79 uses mach files and no dts yet. qca ipq platforms are using dts.
and i have a mikrotik minipcie cards with wireless leds :-)

all 3 ways are valid.
and i have to note that i wrote that patch for embedded devices like 
openwrt does.
this code is a requirement to get the leds to work on several devices 
supported 

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

2018-03-12 Thread Mathias Kresin

10.03.2018 08:56, Sebastian Gottschall:



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?


in our case here it can coexist and will not have a negative effect 
since the led will still run. (except if you reconfigure the gpio to input)

for sure it makes no sense.


Well, it will have negative effects as I noticed in OpenWrt. If the same 
GPIO is controlled by the internal LED function it can't be really used 
via the GPIO controller as there will be "bogus" changes of the GPIO pin 
state.


but however i can block the gpio for beeing reused if the led driver 
took it. this has been made in ath9k for instance.


Most likely I'm not aware of the upstream code you are talking about. 
But OpenWrt has a custom patch which registers the ath9k pins as GPIO 
controller.


As soon as there are pins, manufactures will use them and the assumption 
about a default led connected to a specific pin will fail.


I've seen (home)routers routers using the "default" ath9k LED pin as 
button or for LEDs with a different purpose than "wireless". I never was 
able to find any kind of information which explains why exactly this 
specific pin is used for the ath9k-led. But that's another story.


My solution for OpenWrt was a patch which prevents the creation of the 
ath9k-led if the "default" pin is used as GPIO.


If mach files are used, the ath9k led isn't created (ath9k led pin is 
set to -1) in case the platform data have a button or led using the same 
pin. If the device tree is used, the ath9k led isn't created (ath9k led 
pin is set to -1) if the devicetree node for the ath9k device has the 
gpio-controller property. I'm not really proud of the code and it can be 
most likely done better, but it fixes the issue outlined above.


I've done it this way since the use of the GPIO controller should always 
override any kind of default LEDs.


In the end ath[0|10]k-led is only required for hardware configurations 
where the wireless isn't fixed like with miniPCIe, USB ... wireless 
cards. If the configuration is fixed - like it is for most of the 
(home)routers - the GPIO controller can be registered via the devicetree 
and gpio-leds can be used. Something similar can be most likely done via 
mach files (I barely touch mach files).


I'm aware of the issue since the first version of your patch. But since 
I'm very interested in getting the ath10k pins exposed as gpio 
controller, I planned to add a workaround similar to what we have for 
ath9k to OpenWrt.


Mathias


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

2018-03-09 Thread Sebastian Gottschall



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?


in our case here it can coexist and will not have a negative effect 
since the led will still run. (except if you reconfigure the gpio to input)

for sure it makes no sense.
but however i can block the gpio for beeing reused if the led driver 
took it. this has been made in ath9k for instance.

in ath10k i did not do it, since there is no technical requirement for it.
but that might not be the case for all gpio controllers, so there is no 
generic answer. for ath10k. harmless


Sebastian


Thanks,



--
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.gottsch...@dd-wrt.com
Tel.: +496251-582650 / Fax: +496251-5826565



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

2018-03-09 Thread Julian Calaby
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ł Miłecki:
>>> >>>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 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, _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 code because such code works without
>> libraries, and we are not going to copy 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.cal...@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/


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

2018-03-08 Thread Felix Fietkau
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  
>> >>>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, _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 code because such code works without
> libraries, and we are not going to copy 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.

- Felix


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

2018-03-08 Thread Sebastian Gottschall

Am 08.03.2018 um 16:04 schrieb Pavel Machek:

Hi!


show me a proof that its copy & paste. because its not

I don't have to prove you anything. Sorry.

then i will deny your argument because its false.


But you said:


see ath9k. its exact the same implementation.

We don't want to have exact same code multiple times in the tree.
it isnt the exact same code, its just the way its done. i mean 
registering a led driver function happens multiple times in the kernel
you cannot say that calling led_classdev_register with the required 
parameters and function implementation is a case of code duplication.

then i would just say using of "printk" is a case of code duplication.
registering a led driver is nothing unusual, the implementation of the 
led driver is different each time. the implementation for led_brightness 
is very different

there are many led drivers in the kernel. all are going the same way.

i checked the kernel drivers just right now. almost all major wireless 
drivers are comming with a led driver without using gpiolib
your way is a case of codebloating since a registering gpio-leds 
requires a gpio driver for each wireless driver, even if its sometimes 
just a single register write for a led  and no real gpio.
a gpio driver is more complex and bigger than just a led driver. i just 
wrote a optional gpio driver to get access to all gpios available on the 
card. some vendors are using these in a unusual way
i have seen that vendors used them for reset button input etc. this is 
why i made it. so dont take this as a argument for going a impossible 
way (again. the kernel does not support multiple platform datas with the 
same name
THE KERNEL not leds-gpio. so once a leds-gpio platform data is 
registered, no other driver can register a new one.
in addition the kernel must have gpiolib support which increases the 
kernel size. the best way is always the most simple way and the smallest 
performant way.


and again. i did not duplicate the code of ath9k, i just used it as 
documentation to write a own led driver in a simple way


now a list of wireless drivers with a led driver
intersil
carl9170
ath5k
rt2x00
b43legacy
b43
iwlegacy
rtl8187
brcmsmac
iwlwifi

Sebastian

  
	Pavel



--
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.gottsch...@dd-wrt.com
Tel.: +496251-582650 / Fax: +496251-5826565



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

2018-03-08 Thread Pavel Machek
Hi!

> show me a proof that its copy & paste. because its not

I don't have to prove you anything. Sorry.

But you said:

> >>see ath9k. its exact the same implementation.

We don't want to have exact same code multiple times in the tree.
 
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


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

2018-03-08 Thread Sebastian Gottschall

Am 08.03.2018 um 15:29 schrieb Kalle Valo:

Pavel Machek  writes:


ath10k_leds.gpio = ar->hw_params.led_pin;
gpio_led_register_device(0, _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 code because such code works without
libraries, and we are not going to copy code because that uses
less cache during calls. Sorry.

NAK. Please fix your patch.

To me it's not that black and white, sometimes copying code is simpler
than trying to bring up complicated or restricting frameworks (talking
in general here).

I haven't been able to review this patch in detail yet but from a quick
look most of the code is about standard ath10k infrastructure code. How
many lines of code would using leds-gpio save?
nothing. the led driver code is already very small. (see gpio.c in the 
patch) i had a leds-gpio implementation but it will not work since a 
platform data with the name "leds-gpio" does already exist on various 
platforms (ath79 for instance)
so a second leds-gpio can't be registered. so this solution does simply 
not work and asking me for fixing the kernel generic platform data 
handling like pavel did, is really out of mind
and a own led driver has also better performance than using the 
leds-gpio->gpiolib->ath10k gpio driver callpath.
if someone is still complaining that i added a gpio feature driver as 
well to my patch, i can simply remove that usefull feature and all would 
be happy. but i think having access to all gpios of the card
instead of just the led gpio makes sense to. thats why i implemented a 
gpiolib driver as well


Sebastian


--
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.gottsch...@dd-wrt.com
Tel.: +496251-582650 / Fax: +496251-5826565



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

2018-03-08 Thread Sebastian Gottschall

Am 08.03.2018 um 15:05 schrieb Pavel Machek:

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  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, _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 code because such code works without
libraries, and we are not going to copy code because that uses
less cache during calls. Sorry.

NAK. Please fix your patch.

show me a proof that its copy & paste. because its not


Pavel



--
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.gottsch...@dd-wrt.com
Tel.: +496251-582650 / Fax: +496251-5826565



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

2018-03-08 Thread Kalle Valo
Pavel Machek  writes:

>> >>>ath10k_leds.gpio = ar->hw_params.led_pin;
>> >>>gpio_led_register_device(0, _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 code because such code works without
> libraries, and we are not going to copy code because that uses
> less cache during calls. Sorry.
>
> NAK. Please fix your patch. 

To me it's not that black and white, sometimes copying code is simpler
than trying to bring up complicated or restricting frameworks (talking
in general here).

I haven't been able to review this patch in detail yet but from a quick
look most of the code is about standard ath10k infrastructure code. How
many lines of code would using leds-gpio save?

-- 
Kalle Valo


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

2018-03-08 Thread Pavel Machek
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  
> >>>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, _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 code because such code works without
libraries, and we are not going to copy code because that uses
less cache during calls. Sorry.

NAK. Please fix your patch. 

Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature


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

2018-03-08 Thread Sebastian Gottschall

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  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, _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.
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


Sebastian

Pavel



--
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.gottsch...@dd-wrt.com
Tel.: +496251-582650 / Fax: +496251-5826565



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

2018-03-08 Thread 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  
> >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, _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.
Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature


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

2018-03-07 Thread Sebastian Gottschall

Am 07.03.2018 um 17:22 schrieb Rafał Miłecki:

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 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, _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


Sebastian




--
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.gottsch...@dd-wrt.com
Tel.: +496251-582650 / Fax: +496251-5826565



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

2018-03-07 Thread Rafał Miłecki
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 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, _leds);

-- 
Rafał


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

2018-03-02 Thread Pavel Machek
Hi!

> >>adds also debugfs interface for gpio control.
> >Hi Sebastian,
> >
> >I just noticed this patch and have one question. It seems you register
> >GPIO chip and that WiFi LED is controller by a GPIO. Shouldn't you use
> >leds-gpio driver and just register platform device with
> >gpio_led_platform_data? That way you could avoid some code duplication
> >I think? It seems to be the purpose of leds-gpio driver.

> 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.

NAK.
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature


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

2018-03-02 Thread Sebastian Gottschall

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

Sebastian


NAK.
Pavel



--
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.gottsch...@dd-wrt.com
Tel.: +496251-582650 / Fax: +496251-5826565



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

2018-02-27 Thread Sebastian Gottschall

Am 27.02.2018 um 23:08 schrieb Rafał Miłecki:

On 26 February 2018 at 09:44,   wrote:

From: Sebastian Gottschall 

Adds LED and GPIO Control support for 988x, 9887, 9888, 99x0, 9984 based 
chipsets with on chipset connected led's
using WMI Firmware API.
The LED device will get available named as "ath10k-phyX" at sysfs and can be 
controlled with various triggers.
adds also debugfs interface for gpio control.

Hi Sebastian,

I just noticed this patch and have one question. It seems you register
GPIO chip and that WiFi LED is controller by a GPIO. Shouldn't you use
leds-gpio driver and just register platform device with
gpio_led_platform_data? That way you could avoid some code duplication
I think? It seems to be the purpose of leds-gpio driver.
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.
at the beginning of development of this patch i started in that way and 
found out this limitation, so i had to rewrite everything.
the good point now is, it works even without gpiolib support since the 
gpio device is just a option, but not required for led control.


--
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.gottsch...@dd-wrt.com
Tel.: +496251-582650 / Fax: +496251-5826565



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

2018-02-27 Thread Rafał Miłecki
On 26 February 2018 at 09:44,   wrote:
> From: Sebastian Gottschall 
>
> Adds LED and GPIO Control support for 988x, 9887, 9888, 99x0, 9984 based 
> chipsets with on chipset connected led's
> using WMI Firmware API.
> The LED device will get available named as "ath10k-phyX" at sysfs and can be 
> controlled with various triggers.
> adds also debugfs interface for gpio control.

Hi Sebastian,

I just noticed this patch and have one question. It seems you register
GPIO chip and that WiFi LED is controller by a GPIO. Shouldn't you use
leds-gpio driver and just register platform device with
gpio_led_platform_data? That way you could avoid some code duplication
I think? It seems to be the purpose of leds-gpio driver.


> Signed-off-by: Sebastian Gottschall 
>
> v2  add correct gpio count per chipset and remove IPQ4019 support since this 
> chipset does not make use of specific gpios)
> v5  fix compiling without LED_CLASS and GPIOLIB support, fix also error by 
> kbuild test robot which does not occur in standard builds. curious
> v6  correct return values and fix comment style
> v7  fix ath10k_unregister_led for compiling without LED_CLASS
> v8  fix various code design issues reported by reviewers
> v9  move led and led code to separate sourcefile (gpio.c)
> v10 compile fix if gpiolib isnt included
> v11 make register_gpio_chip static. advise by krobot
> v12 fix warning
> ---
>  drivers/net/wireless/ath/ath10k/Kconfig   |  10 ++
>  drivers/net/wireless/ath/ath10k/Makefile  |   1 +
>  drivers/net/wireless/ath/ath10k/core.c|  28 -
>  drivers/net/wireless/ath/ath10k/core.h|  62 +-
>  drivers/net/wireless/ath/ath10k/debug.c   | 146 ++
>  drivers/net/wireless/ath/ath10k/gpio.c| 196 
> ++
>  drivers/net/wireless/ath/ath10k/hw.h  |   2 +
>  drivers/net/wireless/ath/ath10k/mac.c |   5 +
>  drivers/net/wireless/ath/ath10k/wmi-ops.h |  36 +-
>  drivers/net/wireless/ath/ath10k/wmi-tlv.c |  65 ++
>  drivers/net/wireless/ath/ath10k/wmi.c |  46 +++
>  drivers/net/wireless/ath/ath10k/wmi.h |  36 ++
>  12 files changed, 630 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/net/wireless/ath/ath10k/gpio.c
>
> diff --git a/drivers/net/wireless/ath/ath10k/Kconfig 
> b/drivers/net/wireless/ath/ath10k/Kconfig
> index deb5ae21a559..5d61d499dca4 100644
> --- a/drivers/net/wireless/ath/ath10k/Kconfig
> +++ b/drivers/net/wireless/ath/ath10k/Kconfig
> @@ -10,6 +10,16 @@ config ATH10K
>
>If you choose to build a module, it'll be called ath10k.
>
> +config ATH10K_LEDS
> +   bool "SoftLED Support"
> +   depends on ATH10K
> +   select MAC80211_LEDS
> +   select LEDS_CLASS
> +   select NEW_LEDS
> +   default y
> +   help
> + This option is necessary, if you want LED support for chipset 
> connected led pins
> +
>  config ATH10K_PCI
> tristate "Atheros ath10k PCI support"
> depends on ATH10K && PCI
> diff --git a/drivers/net/wireless/ath/ath10k/Makefile 
> b/drivers/net/wireless/ath/ath10k/Makefile
> index 6739ac26fd29..eccc9806fa43 100644
> --- a/drivers/net/wireless/ath/ath10k/Makefile
> +++ b/drivers/net/wireless/ath/ath10k/Makefile
> @@ -20,6 +20,7 @@ ath10k_core-$(CONFIG_NL80211_TESTMODE) += testmode.o
>  ath10k_core-$(CONFIG_ATH10K_TRACING) += trace.o
>  ath10k_core-$(CONFIG_THERMAL) += thermal.o
>  ath10k_core-$(CONFIG_MAC80211_DEBUGFS) += debugfs_sta.o
> +ath10k_core-$(CONFIG_ATH10K_LEDS) += gpio.o
>  ath10k_core-$(CONFIG_PM) += wow.o
>  ath10k_core-$(CONFIG_DEV_COREDUMP) += coredump.o
>
> diff --git a/drivers/net/wireless/ath/ath10k/core.c 
> b/drivers/net/wireless/ath/ath10k/core.c
> index f3ec13b80b20..d7f89ca98c2d 100644
> --- a/drivers/net/wireless/ath/ath10k/core.c
> +++ b/drivers/net/wireless/ath/ath10k/core.c
> @@ -21,6 +21,10 @@
>  #include 
>  #include 
>  #include 
> +#include 
> +#include 
> +#include 
> +
>
>  #include "core.h"
>  #include "mac.h"
> @@ -65,6 +69,8 @@ static const struct ath10k_hw_params 
> ath10k_hw_params_list[] = {
> .id = QCA988X_HW_2_0_VERSION,
> .dev_id = QCA988X_2_0_DEVICE_ID,
> .name = "qca988x hw2.0",
> +   .led_pin = 1,
> +   .gpio_count = 24,
> .patch_load_addr = QCA988X_HW_2_0_PATCH_LOAD_ADDR,
> .uart_pin = 7,
> .cc_wraparound_type = ATH10K_HW_CC_WRAP_SHIFTED_ALL,
> @@ -94,6 +100,8 @@ static const struct ath10k_hw_params 
> ath10k_hw_params_list[] = {
> .id = QCA988X_HW_2_0_VERSION,
> .dev_id = QCA988X_2_0_DEVICE_ID_UBNT,
> .name = "qca988x hw2.0 ubiquiti",
> +   .led_pin = 1,
> +   .gpio_count = 24,
> .patch_load_addr = QCA988X_HW_2_0_PATCH_LOAD_ADDR,
> .uart_pin = 7,
> .cc_wraparound_type = 

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

2018-02-27 Thread Sebastian Gottschall

Am 27.02.2018 um 18:03 schrieb Steve deRosier:

On Mon, Feb 26, 2018 at 12:44 AM,  wrote:

From: Sebastian Gottschall 

Adds LED and GPIO Control support for 988x, 9887, 9888, 99x0, 9984 based 
chipsets with on chipset connected led's
using WMI Firmware API.
The LED device will get available named as "ath10k-phyX" at sysfs and can be 
controlled with various triggers.
adds also debugfs interface for gpio control.

Signed-off-by: Sebastian Gottschall 

v2  add correct gpio count per chipset and remove IPQ4019 support since this 
chipset does not make use of specific gpios)
v5  fix compiling without LED_CLASS and GPIOLIB support, fix also error by 
kbuild test robot which does not occur in standard builds. curious
v6  correct return values and fix comment style
v7  fix ath10k_unregister_led for compiling without LED_CLASS
v8  fix various code design issues reported by reviewers
v9  move led and led code to separate sourcefile (gpio.c)
v10 compile fix if gpiolib isnt included
v11 make register_gpio_chip static. advise by krobot
v12 fix warning
---
  drivers/net/wireless/ath/ath10k/Kconfig   |  10 ++
  drivers/net/wireless/ath/ath10k/Makefile  |   1 +
  drivers/net/wireless/ath/ath10k/core.c|  28 -
  drivers/net/wireless/ath/ath10k/core.h|  62 +-
  drivers/net/wireless/ath/ath10k/debug.c   | 146 ++
  drivers/net/wireless/ath/ath10k/gpio.c| 196 ++
  drivers/net/wireless/ath/ath10k/hw.h  |   2 +
  drivers/net/wireless/ath/ath10k/mac.c |   5 +
  drivers/net/wireless/ath/ath10k/wmi-ops.h |  36 +-
  drivers/net/wireless/ath/ath10k/wmi-tlv.c |  65 ++
  drivers/net/wireless/ath/ath10k/wmi.c |  46 +++
  drivers/net/wireless/ath/ath10k/wmi.h |  36 ++
  12 files changed, 630 insertions(+), 3 deletions(-)
  create mode 100644 drivers/net/wireless/ath/ath10k/gpio.c


Assuming that kbuild robot doesn't kick back another build-time
warning, it looks OK to me.

:-) seems so


Reviewed-by: Steve deRosier 

--
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.gottsch...@dd-wrt.com
Tel.: +496251-582650 / Fax: +496251-5826565



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

2018-02-27 Thread Steve deRosier
On Mon, Feb 26, 2018 at 12:44 AM,  wrote:
>
> From: Sebastian Gottschall 
>
> Adds LED and GPIO Control support for 988x, 9887, 9888, 99x0, 9984 based 
> chipsets with on chipset connected led's
> using WMI Firmware API.
> The LED device will get available named as "ath10k-phyX" at sysfs and can be 
> controlled with various triggers.
> adds also debugfs interface for gpio control.
>
> Signed-off-by: Sebastian Gottschall 
>
> v2  add correct gpio count per chipset and remove IPQ4019 support since this 
> chipset does not make use of specific gpios)
> v5  fix compiling without LED_CLASS and GPIOLIB support, fix also error by 
> kbuild test robot which does not occur in standard builds. curious
> v6  correct return values and fix comment style
> v7  fix ath10k_unregister_led for compiling without LED_CLASS
> v8  fix various code design issues reported by reviewers
> v9  move led and led code to separate sourcefile (gpio.c)
> v10 compile fix if gpiolib isnt included
> v11 make register_gpio_chip static. advise by krobot
> v12 fix warning
> ---
>  drivers/net/wireless/ath/ath10k/Kconfig   |  10 ++
>  drivers/net/wireless/ath/ath10k/Makefile  |   1 +
>  drivers/net/wireless/ath/ath10k/core.c|  28 -
>  drivers/net/wireless/ath/ath10k/core.h|  62 +-
>  drivers/net/wireless/ath/ath10k/debug.c   | 146 ++
>  drivers/net/wireless/ath/ath10k/gpio.c| 196 
> ++
>  drivers/net/wireless/ath/ath10k/hw.h  |   2 +
>  drivers/net/wireless/ath/ath10k/mac.c |   5 +
>  drivers/net/wireless/ath/ath10k/wmi-ops.h |  36 +-
>  drivers/net/wireless/ath/ath10k/wmi-tlv.c |  65 ++
>  drivers/net/wireless/ath/ath10k/wmi.c |  46 +++
>  drivers/net/wireless/ath/ath10k/wmi.h |  36 ++
>  12 files changed, 630 insertions(+), 3 deletions(-)
>  create mode 100644 drivers/net/wireless/ath/ath10k/gpio.c


Assuming that kbuild robot doesn't kick back another build-time
warning, it looks OK to me.

Reviewed-by: Steve deRosier 

--
Steve deRosier
Cal-Sierra Consulting LLC
https://www.cal-sierra.com/