Skip to content

Conversation

@MaxAgneni
Copy link

This is part of a patch series to add support for winc1500 wifi module

origin: extracted from Atmel ASF version 3.33.0

was: https://gerrit.zephyrproject.org/r/#/c/12356/

@tbursztyka tbursztyka self-requested a review May 3, 2017 07:24
@jukkar jukkar self-requested a review May 3, 2017 07:51
Copy link
Contributor

@tbursztyka tbursztyka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is the NET_DEVICE_INIT or at least DEVICE_INIT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change this into a menuconfig WIFI directly

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and you can avoid the prompt by doing: bool "Add support for WiFi drivers"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and then here you would do:

if WIFI

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and here, instead of endmenu:

endif # WIFI

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like this file should go somewhere in ext/ or?

What goes into all directories but ext, is made for zephyr so there is no such thing as redefining stuff like NULL, uints ... etc... that's useless: Zephyr provides all necessary bits and pieces.

Copy link
Member

@jukkar jukkar May 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with Tomasz, these changes look like they belong to ext.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left the function prototypes in this file and moved the other definitions in ext

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so no camelcase, but change this to:

K_MS(time_msec) * USEC_PER_SEC

USEC_PER_SEC is found in include/sys_clock.h

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why redefining this??

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to re-document how spi is configured as all is in spi.h already

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed and configured with spi defines

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that does not seem to be the case. You are returning -1 in error.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

80 chars limit.

Copy link
Member

@jukkar jukkar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please have a proper commit messages. So subject, body and signed-off etc.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No typedefs for structs.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

Copy link
Member

@jukkar jukkar May 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with Tomasz, these changes look like they belong to ext.

Copy link
Contributor

@tbursztyka tbursztyka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And please have a proper commit message.

Signed-off-by etc... and we don't care of former gerrit url.

@MaxAgneni MaxAgneni force-pushed the net branch 2 times, most recently from 4980ace to 834c9f2 Compare May 10, 2017 07:16
@jukkar jukkar added the net label May 15, 2017
@nashif nashif changed the base branch from net to master May 30, 2017 11:57
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this, this board does not use NBLE, NBLE is only being used for Arduino 101 with a custom Bluetooth firmware

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove the whole section above

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part of Kconfig is just removed

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

everything is commented out here, so what is the point of adding this file?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

point to add this file is drivers/wifi/winc1500/Kconfig

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but the while thing is commented, so it has no effect at all

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in Makefile there is:
obj-$(CONFIG_WIFI_WINC1500) += ....

pnndra added 5 commits July 3, 2017 20:48
This is part of a patch series to add support for winc1500 wifi module

origin: extracted from Atmel ASF version 3.33.0

Signed-off-by: Massimiliano Agneni <[email protected]>
Signed-off-by: Anas Nashif <[email protected]>
Signed-off-by: Dario Pennisi <[email protected]>
was: https://gerrit.zephyrproject.org/r/#/c/12365/

Signed-off-by: Massimiliano Agneni <[email protected]>
Signed-off-by: Anas Nashif <[email protected]>
Signed-off-by: Dario Pennisi <[email protected]>
Signed-off-by: Massimiliano Agneni <[email protected]>
Signed-off-by: Dario Pennisi <[email protected]>
Signed-off-by: Anas Nashif <[email protected]>
was: https://gerrit.zephyrproject.org/r/#/c/12364/

Signed-off-by: Massimiliano Agneni <[email protected]>
Signed-off-by: Dario Pennisi <[email protected]>
Signed-off-by: Anas Nashif <[email protected]>
@drahnr
Copy link

drahnr commented Sep 7, 2017

Is there a general need for the offload ip stack with this driver? The last commit only removes a couple of dependencies, but not all of them.

@galak
Copy link
Contributor

galak commented Nov 3, 2017

Taken over by #4711

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants