feedback on mainlining wilc1000 staging driver
Hi Greg,
We all are working on submitting and reviewing patches for wilc1000 in staging driver for quite some time.
We would like to have feedback on the next steps to bring wilc1000 driver closer to move into the wireless subsystem tree.
In summary, the following major things from TODO have been addressed in staging:
-remove the defined feature as kernel versions
-remove OS wrapper functions
-remove custom debug and tracing functions
-rework comments and function headers(also coding style) -remove build warnings
-replace all semaphores with mutexes or completions
-make spi and sdio components coexist in one build
-turn compile-time platform configuration (BEAGLE_BOARD, PANDA_BOARD, PLAT_WMS8304, PLAT_RKXXXX, CUSTOMER_PLATFORM, ...) into run-time options that are read from DT
-replace SIOCDEVPRIVATE commands with generic API functions -use wext-core handling instead of private SIOCSIWPRIV implementation -Move handling for each individual members of 'union message_body' out into a separate 'struct work_struct' and completely remove the multiplexer that is currently part of host_if_work(), allowing movement of the implementation of each message handler into the callsite of the function that currently queues the 'host_if_msg'.
-convert all uses of the old GPIO API from <linux/gpio.h> to the GPIO descriptor API in <linux/gpio/consumer.h> and look up GPIO lines from device tree, ACPI or board files, board files should use <linux/gpio/machine.h>
I have few patches to submit based on last week comments. I will send these patches as soon as the merge window is closed.
1. Delete wilc_debugfs.c file, as not used.
2. Remove use of remaining global variables.
3. Remove udelay realted checkpatch warning.
4. Use void return for function whose return value is not used.
Currently, there are 2 known items pending in our TODO. -support soft-ap and p2p mode
-support resume/suspend function
For the above 2 remaining TODO items, we need to add extra code but as suggested in [1], we have not taken up this activity yet.
We would like to know your opinion whether we should start working on these features in staging-next tree or take it up after having moved the code to wireless sub-system.
Do you think it’s better to initiate review in parallel with wireless subsystem maintainer to identify any pending TODO items.
Please guide us on how to proceed further.
[1] . https://patchwork.kernel.org/patch/9809145
Regards,
Ajay
Quoted 7 lines · Ajay Singh
Hi Greg,
We all are working on submitting and reviewing patches for wilc1000 in staging driver for quite some time.
We would like to have feedback on the next steps to bring wilc1000 driver closer to move into the wireless subsystem tree.
<snip>
It is the middle of the merge window, combined with all of the 1ltf crap, and other stable stuff. Can you wait until after the merge window for this please? There's nothing we can do now anyway. Submit the rest of your pending patches, get those merged and then we can review the code to see what you have left to do.
thanks,
greg k-h
Hi Greg,
Quoted 9 lines · On Thu, 16 Aug 2018 08:43:54 +0200 Greg KH <[email protected]> wrote:
Quoted 7 lines · Ajay Singh
Hi Greg,
We all are working on submitting and reviewing patches for wilc1000 in staging driver for quite some time.
We would like to have feedback on the next steps to bring wilc1000 driver closer to move into the wireless subsystem tree.
Quoted 8 lines
<snip>
It is the middle of the merge window, combined with all of the 1ltf crap, and other stable stuff. Can you wait until after the merge window for this please? There's nothing we can do now anyway. Submit the rest of your pending patches, get those merged and then we can review the code to see what you have left to do.
Thanks for your time.
I can wait till the end of merge window to discuss this. My intention is not to send changes during the current merge window but to understand what is pending for our driver.
Let's revisit this later as you have suggested.
Regards,
Ajay
Quoted 23 lines · Ajay Singh
Hi Greg,
We all are working on submitting and reviewing patches for wilc1000 in staging driver for quite some time.
We would like to have feedback on the next steps to bring wilc1000 driver closer to move into the wireless subsystem tree.
In summary, the following major things from TODO have been addressed in staging:
(quoted diff) +0
-8-remove the defined feature as kernel versions -remove OS wrapper functions -remove custom debug and tracing functions -rework comments and function headers(also coding style) -remove build warnings -replace all semaphores with mutexes or completions -make spi and sdio components coexist in one build -turn compile-time platform configuration (BEAGLE_BOARD, PANDA_BOARD,
PLAT_WMS8304,PLAT_RKXXXX,CUSTOMER_PLATFORM, ...) into run-time options that are read from DT(quoted diff) +0
-2-replace SIOCDEVPRIVATE commands with generic API functions -use wext-core handling instead of private SIOCSIWPRIV implementation
Quoted 1 line
From wireless point of view: if I see wext mentioned anywhere in the
driver I stop right there. cfg80211 is a hard requirement for us nowadays.
Signature · 1 line
Kalle Valo
Quoted 20 lines · Kalle Valo
Quoted 18 lines · Ajay Singh
Hi Greg,
We all are working on submitting and reviewing patches for wilc1000 in staging driver for quite some time.
We would like to have feedback on the next steps to bring wilc1000 driver closer to move into the wireless subsystem tree.
In summary, the following major things from TODO have been addressed in staging:
(quoted diff) +0
-8-remove the defined feature as kernel versions -remove OS wrapper functions -remove custom debug and tracing functions -rework comments and function headers(also coding style) -remove build warnings -replace all semaphores with mutexes or completions -make spi and sdio components coexist in one build -turn compile-time platform configuration (BEAGLE_BOARD, PANDA_BOARD,
Quoted 8 lines
Review
Quoted 4 lines
PLAT_WMS8304,PLAT_RKXXXX,CUSTOMER_PLATFORM, ...) into run-time options that are read from DT(quoted diff) +0
-2-replace SIOCDEVPRIVATE commands with generic API functions -use wext-core handling instead of private SIOCSIWPRIV implementationFrom wireless point of view: if I see wext mentioned anywhere in the driver I stop right there. cfg80211 is a hard requirement for us nowadays.
Clarification: Depending on the hardware design either cfg80211 or mac80211 is a hard requirement. I haven't checked wilc1000 at all so I don't know what design it has but if it's a "softmac" design then it has to use mac80211, we do not want to support multiple 802.11 UMAC stacks.
Signature · 1 line
Kalle Valo
Quoted 32 lines · On Thu, 16 Aug 2018 13:53:50 +0300 Kalle Valo <[email protected]> wrote:
Quoted 21 lines · Kalle Valo
Quoted 19 lines · Ajay Singh
Hi Greg,
We all are working on submitting and reviewing patches for wilc1000 in staging driver for quite some time.
We would like to have feedback on the next steps to bring wilc1000 driver closer to move into the wireless subsystem tree.
In summary, the following major things from TODO have been addressed in staging:
(quoted diff) +0
-8-remove the defined feature as kernel versions -remove OS wrapper functions -remove custom debug and tracing functions -rework comments and function headers(also coding style) -remove build warnings -replace all semaphores with mutexes or completions -make spi and sdio components coexist in one build -turn compile-time platform configuration (BEAGLE_BOARD,
PANDA_BOARD,PLAT_WMS8304,PLAT_RKXXXX,CUSTOMER_PLATFORM, ...)Quoted 8 lines
Quoted 4 lines
into run-time options that are read from DT
(quoted diff) +0
-2-replace SIOCDEVPRIVATE commands with generic API functions -use wext-core handling instead of private SIOCSIWPRIVimplementation
From wireless point of view: if I see wext mentioned anywhere in the driver I stop right there. cfg80211 is a hard requirement for us nowadays.
Quoted 6 lines
Clarification: Depending on the hardware design either cfg80211 or mac80211 is a hard requirement. I haven't checked wilc1000 at all so I don't know what design it has but if it's a "softmac" design then it has to use mac80211, we do not want to support multiple 802.11 UMAC stacks.
The TODO item to make use of wext-core is obsolete. Previously wilc1000 driver also had few wext ioctl use but now it’s completely removed and cfg80211 operation callbacks are used.
wilc1000 driver make use of cfg80211 and isn’t a "softmac".
We need help to review and identify if there are any pending items for wilc1000 driver, so we can address those issues and make it ready to move to the wireless subsystem.
Regards,
Ajay
Quoted 42 lines · Ajay Singh
Quoted 31 lines · On Thu, 16 Aug 2018 13:53:50 +0300 Kalle Valo <[email protected]> wrote:
Kalle Valo <[email protected]> writes:
=20Quoted 21 lines
Ajay Singh <[email protected]> writes:
=20=20Quoted 19 lines
Hi Greg,
We all are working on submitting and reviewing patches for wilc1000 in staging driver for quite some time.=20
We would like to have feedback on the next steps to bring wilc1000 driver closer to move into the wireless subsystem tree.=20
In summary, the following major things from TODO have been addressed in staging:
(quoted diff) +0
-8-remove the defined feature as kernel versions -remove OS wrapper functions -remove custom debug and tracing functions -rework comments and function headers(also coding style) -remove build warnings -replace all semaphores with mutexes or completions -make spi and sdio components coexist in one build -turn compile-time platform configuration (BEAGLE_BOARD,
PANDA_BOARD,PLAT_WMS8304,PLAT_RKXXXX,CUSTOMER_PLATFORM, ...)Quoted 8 lines
Quoted 4 lines
into run-time options that are read from DT
(quoted diff) +0
-2-replace SIOCDEVPRIVATE commands with generic API functions -use wext-core handling instead of private SIOCSIWPRIVimplementation=20=20
From wireless point of view: if I see wext mentioned anywhere in the driver I stop right there. cfg80211 is a hard requirement for us nowadays.=20=20
Quoted 7 lines
=20
Clarification: Depending on the hardware design either cfg80211 or mac80211 is a hard requirement. I haven't checked wilc1000 at all so I don't know what design it has but if it's a "softmac" design then it has to use mac80211, we do not want to support multiple 802.11 UMAC stacks.
=20
Quoted 2 lines
The TODO item to make use of wext-core is obsolete. Previously wilc1000 driver also had few wext ioctl use but now it=E2=80=99s completely remove=
d and
Quoted 3 lines
cfg80211 operation callbacks are used.
wilc1000 driver make use of cfg80211 and isn=E2=80=99t a "softmac".
Good.
Quoted 3 lines
We need help to review and identify if there are any pending items for wilc1000 driver, so we can address those issues and make it ready to move to the wireless subsystem.
I think the best way to get that forward is to submit a patch (or patchset) to linux-wireless, that's the easiest for reviewers.
--=20
Kalle Valo
Quoted 50 lines · Kalle Valo
Quoted 42 lines · Ajay Singh
Quoted 32 lines · On Thu, 16 Aug 2018 13:53:50 +0300 Kalle Valo <[email protected]> wrote:
Quoted 21 lines · Kalle Valo
Quoted 19 lines · Ajay Singh
Hi Greg,
We all are working on submitting and reviewing patches for wilc1000 in staging driver for quite some time.
We would like to have feedback on the next steps to bring wilc1000 driver closer to move into the wireless subsystem tree.
In summary, the following major things from TODO have been addressed in staging:
(quoted diff) +0
-8-remove the defined feature as kernel versions -remove OS wrapper functions -remove custom debug and tracing functions -rework comments and function headers(also coding style) -remove build warnings -replace all semaphores with mutexes or completions -make spi and sdio components coexist in one build -turn compile-time platform configuration (BEAGLE_BOARD,
PANDA_BOARD,PLAT_WMS8304,PLAT_RKXXXX,CUSTOMER_PLATFORM, ...)Quoted 8 lines
Quoted 4 lines
into run-time options that are read from DT
(quoted diff) +0
-2-replace SIOCDEVPRIVATE commands with generic API functions -use wext-core handling instead of private SIOCSIWPRIVimplementation
From wireless point of view: if I see wext mentioned anywhere in the
driver I stop right there. cfg80211 is a hard requirement for us nowadays.Quoted 6 lines
Clarification: Depending on the hardware design either cfg80211 or mac80211 is a hard requirement. I haven't checked wilc1000 at all so I don't know what design it has but if it's a "softmac" design then it has to use mac80211, we do not want to support multiple 802.11 UMAC stacks.
Quoted 5 lines
The TODO item to make use of wext-core is obsolete. Previously wilc1000 driver also had few wext ioctl use but now it’s completely removed and cfg80211 operation callbacks are used.
wilc1000 driver make use of cfg80211 and isn’t a "softmac".
Quoted 8 lines
Good.
Quoted 3 lines
We need help to review and identify if there are any pending items for wilc1000 driver, so we can address those issues and make it ready to move to the wireless subsystem.
I think the best way to get that forward is to submit a patch (or patchset) to linux-wireless, that's the easiest for reviewers.
For brcm80211 drivers we used a single patch introducing it under the wireless drivers folder. Because it was quite a sizable patch we parked it on the wireless wiki page. Had a few iterations doing it like that.
Regards,
Arend
Quoted 19 lines · Arend van Spriel
Quoted 18 lines · Kalle Valo
Quoted 16 lines · Ajay Singh
Quoted 12 lines · On Thu, 16 Aug 2018 13:53:50 +0300 Kalle Valo <[email protected]> wrote:
Quoted 3 lines · Kalle Valo
From wireless point of view: if I see wext mentioned anywhere in the
driver I stop right there. cfg80211 is a hard requirement for us nowadays.Clarification: Depending on the hardware design either cfg80211 or mac80211 is a hard requirement. I haven't checked wilc1000 at all so I don't know what design it has but if it's a "softmac" design then it has to use mac80211, we do not want to support multiple 802.11 UMAC stacks.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
The TODO item to make use of wext-core is obsolete. Previously wilc1000 driver also had few wext ioctl use but now it=E2=80=99s completely remo=
ved and
Quoted 10 lines
Quoted 10 lines
Quoted 3 lines
cfg80211 operation callbacks are used.
wilc1000 driver make use of cfg80211 and isn=E2=80=99t a "softmac".
Good.
Quoted 3 lines
We need help to review and identify if there are any pending items for wilc1000 driver, so we can address those issues and make it ready to move to the wireless subsystem.
Quoted 7 lines
Quoted 2 lines
I think the best way to get that forward is to submit a patch (or patchset) to linux-wireless, that's the easiest for reviewers.
For brcm80211 drivers we used a single patch introducing it under the wireless drivers folder. Because it was quite a sizable patch we parked it on the wireless wiki page. Had a few iterations doing it like that.
Another option is to split it so that there's one patch per file, should be even pretty easy to automate that. It's just so much easier to comment on a patch submitted by email compared to the reviewer manually copying code and then commenting it, yuck.
--=20
Kalle Valo
Quoted 41 lines · On Fri, 17 Aug 2018 11:36:02 +0300 Kalle Valo <[email protected]> wrote:
Quoted 31 lines · Arend van Spriel
Quoted 30 lines · Kalle Valo
Quoted 16 lines · Ajay Singh
Quoted 6 lines · On Thu, 16 Aug 2018 13:53:50 +0300 Kalle Valo <[email protected]> wrote:
Quoted 3 lines · Kalle Valo
From wireless point of view: if I see wext mentioned anywhere
in the driver I stop right there. cfg80211 is a hard requirement for us nowadays.Quoted 6 lines
Clarification: Depending on the hardware design either cfg80211 or mac80211 is a hard requirement. I haven't checked wilc1000 at all so I don't know what design it has but if it's a "softmac" design then it has to use mac80211, we do not want to support multiple 802.11 UMAC stacks.
Quoted 5 lines
The TODO item to make use of wext-core is obsolete. Previously wilc1000 driver also had few wext ioctl use but now it’s completely removed and cfg80211 operation callbacks are used.
wilc1000 driver make use of cfg80211 and isn’t a "softmac".
Good.
Quoted 3 lines
We need help to review and identify if there are any pending items for wilc1000 driver, so we can address those issues and make it ready to move to the wireless subsystem.
Quoted 7 lines
Quoted 2 lines
I think the best way to get that forward is to submit a patch (or patchset) to linux-wireless, that's the easiest for reviewers.
For brcm80211 drivers we used a single patch introducing it under the wireless drivers folder. Because it was quite a sizable patch we parked it on the wireless wiki page. Had a few iterations doing it like that.
Quoted 5 lines
Another option is to split it so that there's one patch per file, should be even pretty easy to automate that. It's just so much easier to comment on a patch submitted by email compared to the reviewer manually copying code and then commenting it, yuck.
Sure. I will prepare a patch per file send for review as its easy to review.
As Greg suggested, I will wait for the merge window to close and after completing pending patches to staging, I will start the review.
For my understanding, the patches for review will be based on wireless-testing branch. And the fixes will be submitted to staging tree in parallel. right?
Regards,
Ajay
Quoted 18 lines · Ajay Singh
Quoted 12 lines
Quoted 11 lines
Quoted 6 lines
Quoted 3 lines
We need help to review and identify if there are any pending items for wilc1000 driver, so we can address those issues and make it ready to move to the wireless subsystem.
I think the best way to get that forward is to submit a patch (or patchset) to linux-wireless, that's the easiest for reviewers.
For brcm80211 drivers we used a single patch introducing it under the wireless drivers folder. Because it was quite a sizable patch we parked it on the wireless wiki page. Had a few iterations doing it like that.
Quoted 5 lines
Another option is to split it so that there's one patch per file, should be even pretty easy to automate that. It's just so much easier to comment on a patch submitted by email compared to the reviewer manually copying code and then commenting it, yuck.
Quoted 8 lines
Sure. I will prepare a patch per file send for review as its easy to review.
As Greg suggested, I will wait for the merge window to close and after completing pending patches to staging, I will start the review.
For my understanding, the patches for review will be based on wireless-testing branch.
In this case I think wireless-drivers-next is the safest choise, wireless-testing also has other trees which might cause conflicts etc.
Quoted 1 line
And the fixes will be submitted to staging tree in parallel. right?
I don't think the fixes matter in the initial review. So yeah, Greg can apply fixes as he sees fit for now. At some point we need to do a cut off period and freeze the driver for the last review round before it will be (hopefully) moved to drivers/net/wireless, but let's coordinate with Greg once we are at that stage.
Signature · 1 line
Kalle Valo
Hi Kalle,
Quoted 28 lines · On Thu, 23 Aug 2018 16:37:45 +0530 Kalle Valo <[email protected]> wrote:
Quoted 26 lines · Ajay Singh
Quoted 13 lines
Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
We need help to review and identify if there are any pending items for wilc1000 driver, so we can address those issues and make it ready to move to the wireless subsystem.
Quoted 8 lines
Quoted 3 lines
I think the best way to get that forward is to submit a patch (or patchset) to linux-wireless, that's the easiest for reviewers.
For brcm80211 drivers we used a single patch introducing it under the wireless drivers folder. Because it was quite a sizable patch we parked it on the wireless wiki page. Had a few iterations doing it like that.
Quoted 5 lines
Another option is to split it so that there's one patch per file, should be even pretty easy to automate that. It's just so much easier to comment on a patch submitted by email compared to the reviewer manually copying code and then commenting it, yuck.
Sure. I will prepare a patch per file send for review as its easy to review.
As Greg suggested, I will wait for the merge window to close and after completing pending patches to staging, I will start the review.
Quoted 5 lines
Quoted 2 lines
For my understanding, the patches for review will be based on wireless-testing branch.
In this case I think wireless-drivers-next is the safest choise, wireless-testing also has other trees which might cause conflicts etc.
I have submitted a patch series for wilc1000 driver, single file per patch and its based on wireless-drivers-next.
I hope its done correctly, please provide inputs so we can address and make this driver ready for mainline.
Regards,
Ajay
Quoted 36 lines · Ajay Singh
Hi Kalle,
Quoted 31 lines · On Thu, 23 Aug 2018 16:37:45 +0530 Kalle Valo <[email protected]> wrote:
Quoted 28 lines · Ajay Singh
Quoted 13 lines
Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
We need help to review and identify if there are any pending items for wilc1000 driver, so we can address those issues and make it ready to move to the wireless subsystem.
Quoted 8 lines
Quoted 3 lines
I think the best way to get that forward is to submit a patch (or patchset) to linux-wireless, that's the easiest for reviewers.
For brcm80211 drivers we used a single patch introducing it under the wireless drivers folder. Because it was quite a sizable patch we parked it on the wireless wiki page. Had a few iterations doing it like that.
Quoted 5 lines
Another option is to split it so that there's one patch per file, should be even pretty easy to automate that. It's just so much easier to comment on a patch submitted by email compared to the reviewer manually copying code and then commenting it, yuck.
Sure. I will prepare a patch per file send for review as its easy to review.
As Greg suggested, I will wait for the merge window to close and after completing pending patches to staging, I will start the review.
For my understanding, the patches for review will be based on wireless-testing branch.
Quoted 7 lines
Quoted 2 lines
In this case I think wireless-drivers-next is the safest choise, wireless-testing also has other trees which might cause conflicts etc.
I have submitted a patch series for wilc1000 driver, single file per patch and its based on wireless-drivers-next.
I hope its done correctly, please provide inputs so we can address and make this driver ready for mainline.
Thanks, I see it in patchwork:
https://patchwork.kernel.org/project/linux-wireless/list/?series=23251&state=*&order=date
Do note that we have also another new driver (rtwlan/rtw88) under review so review from the wireless folks might take some time.
Signature · 1 line
Kalle Valo
Hi Kalle,
Quoted 47 lines · On Thu, 4 Oct 2018 15:27:57 +0300 Kalle Valo <[email protected]> wrote:
Quoted 36 lines · Ajay Singh
Hi Kalle,
Quoted 31 lines · On Thu, 23 Aug 2018 16:37:45 +0530 Kalle Valo <[email protected]> wrote:
Quoted 28 lines · Ajay Singh
Quoted 13 lines
Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
We need help to review and identify if there are any pending items for wilc1000 driver, so we can address those issues and make it ready to move to the wireless subsystem.
Quoted 8 lines
Quoted 3 lines
I think the best way to get that forward is to submit a patch (or patchset) to linux-wireless, that's the easiest for reviewers.
For brcm80211 drivers we used a single patch introducing it under the wireless drivers folder. Because it was quite a sizable patch we parked it on the wireless wiki page. Had a few iterations doing it like that.
Quoted 5 lines
Another option is to split it so that there's one patch per file, should be even pretty easy to automate that. It's just so much easier to comment on a patch submitted by email compared to the reviewer manually copying code and then commenting it, yuck.
Sure. I will prepare a patch per file send for review as its easy to review.
As Greg suggested, I will wait for the merge window to close and after completing pending patches to staging, I will start the review.
For my understanding, the patches for review will be based on wireless-testing branch.
Quoted 8 lines
Quoted 3 lines
In this case I think wireless-drivers-next is the safest choise, wireless-testing also has other trees which might cause conflicts etc.
I have submitted a patch series for wilc1000 driver, single file per patch and its based on wireless-drivers-next.
I hope its done correctly, please provide inputs so we can address and make this driver ready for mainline.
Quoted 7 lines
Thanks, I see it in patchwork:
https://patchwork.kernel.org/project/linux-wireless/list/?series=23251&state=*&order=date
Do note that we have also another new driver (rtwlan/rtw88) under review so review from the wireless folks might take some time.
Thanks for the update.
Btw I could see our driver was added to the pending branch. Just curious, could you please share details about how the pending branch is used ?
Regards,
Ajay
Quoted 9 lines · Ajay Singh
Quoted 9 lines
Quoted 4 lines
I have submitted a patch series for wilc1000 driver, single file per patch and its based on wireless-drivers-next.
I hope its done correctly, please provide inputs so we can address and make this driver ready for mainline.Thanks, I see it in patchwork:
https://patchwork.kernel.org/project/linux-wireless/list/?series=23251&state=*&order=date
Quoted 8 lines
Quoted 3 lines
Do note that we have also another new driver (rtwlan/rtw88) under review so review from the wireless folks might take some time.
Thanks for the update.
Btw I could see our driver was added to the pending branch. Just curious, could you please share details about how the pending branch is used ?
I use pending branch mainly for testing bigger patches with kbuild bot, though I think the bot is offline this week. (Or was it next week? Can't remember)
Signature · 1 line
Kalle Valo