Skip to content

net: change enc28j60 to new SPI API #662

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

net: change enc28j60 to new SPI API #662

wants to merge 2 commits into from

Conversation

locomuco
Copy link
Contributor

@locomuco locomuco commented Jul 1, 2017

Signed-off-by: Matthias Boesl [email protected]

@locomuco locomuco mentioned this pull request Jul 1, 2017
Closed
@locomuco locomuco changed the title net: change enc28j60 to new SPI API [WIP] net: change enc28j60 to new SPI API Jul 1, 2017
@jukkar jukkar requested a review from tbursztyka July 1, 2017 14:30
@jukkar jukkar added the net label Jul 1, 2017
tbursztyka
tbursztyka requested changes Jul 5, 2017
View reviewed changes
Copy link
Collaborator

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

Apply also the necessary changes pin-pointed by checkpatch (80chars limit etc...)

drivers/ethernet/eth_enc28j60.c Outdated
@@ -43,12 +50,19 @@ static void eth_enc28j60_set_bank(struct device *dev, u16_t reg_addr)
tx_buf[0] = ENC28J60_SPI_RCR | ENC28J60_REG_ECON1;
tx_buf[1] = 0x0;

spi_transceive(context->spi, tx_buf, 2, tx_buf, 2);
struct spi_buf tx_bufs[] = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Always declare variables at the beginning of a code block, not in the middle, so move this up.
Apply that rule everywhere.

@locomuco locomuco force-pushed the enc branch 8 times, most recently from e01c086 to d4ba099 Compare July 9, 2017 08:11
@locomuco locomuco force-pushed the enc branch 3 times, most recently from 58d83a8 to 3ef1822 Compare July 15, 2017 10:13
@locomuco locomuco requested a review from jukkar as a code owner September 5, 2017 06:02
@locomuco locomuco changed the title [WIP] net: change enc28j60 to new SPI API net: change enc28j60 to new SPI API Sep 5, 2017
@locomuco
Copy link
Contributor Author

locomuco commented Sep 5, 2017

@tbursztyka the PR is updated to support legacy and new SPI interface now

tbursztyka
tbursztyka requested changes Sep 5, 2017
View reviewed changes
drivers/spi/Kconfig Outdated
@@ -25,7 +25,7 @@ menuconfig SPI

config SPI_QMSI
bool "QMSI driver for SPI controller"
depends on SPI && QMSI
depends on SPI && QMSI && SPI_LEGACY_API
Copy link
Collaborator

Choose a reason for hiding this comment

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

These Kconfig fixes should be in another patch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what's platforms are exactly tested in the shippable builds for the enc28j60?

drivers/ethernet/eth_enc28j60_priv.h
@@ -229,7 +231,7 @@ struct eth_enc28j60_runtime {
CONFIG_ETH_ENC28J60_RX_THREAD_STACK_SIZE);
struct k_thread thread;
struct device *gpio;
struct device *spi;
struct spi_config spi_cfg;
struct gpio_callback gpio_cb;
Copy link
Collaborator

Choose a reason for hiding this comment

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

You will need #ifdef/#else/#endif instead of removing *spi

drivers/ethernet/eth_enc28j60.c Outdated
config->gpio_pin);
return -EINVAL;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

You don't need to configure the gpio cs by yourself, neither in spi legacy nor the new one.

drivers/ethernet/eth_enc28j60.c Outdated
context->spi_cfg.vendor = 1;
context->spi_cfg.cs = &enc_spi_cs;
enc_spi_cs.gpio_pin = config->spi_cs_pin;

Copy link
Collaborator

Choose a reason for hiding this comment

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

this will need no handle the case when config->spi_cs_port is just "" (aka: undefined), which means there is no need of enc_spi_cs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do

@locomuco locomuco force-pushed the enc branch 2 times, most recently from 5f04862 to d3c3fc9 Compare September 5, 2017 07:38
@zephyrproject-rtos zephyrproject-rtos deleted a comment from locomuco Sep 5, 2017
tbursztyka
tbursztyka reviewed Sep 5, 2017
View reviewed changes
drivers/ethernet/eth_enc28j60.c Outdated
return -EINVAL;
}
#endif

Copy link
Collaborator

Choose a reason for hiding this comment

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

[HOW COME github let me delete your comment, by mistake? how smart is this... sorry ]

spi_context_cs_configure() should do it, on spi driver side.

Could it be your board needs something specific? I see this GPIO_PUD_PULL_DOWN, looks like it's specific. (it's not supported by all gpio controller)

I could make a quick patch so extra gpio directives can be passed to gpio_pin_configure() in drivers/spi/spi_context.h

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no problem :-) , i might move that part into my board folder, thanks

@locomuco locomuco force-pushed the enc branch 2 times, most recently from 4dd2cf5 to e484ecb Compare September 5, 2017 17:21
@locomuco
Copy link
Contributor Author

locomuco commented Sep 5, 2017

@tbursztyka now everything should be addressed

tbursztyka
tbursztyka previously approved these changes Sep 7, 2017
View reviewed changes
Copy link
Collaborator

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

Let's call it a go.

When legacy API will be finally dropped, it would be interesting to change the logic of writing/reading a packet in enc28j60's memory: trying to make it in one go, instead of looping currently by MAX_BUFFER_LENGTH (in "segments"). New spi api is exactly meant for this: avoiding multiple calls and memory copies.
Doing it right now would entangle way more #ifdefs everywhere so let's postpone this.

@nashif nashif added this to the v1.10 milestone Sep 7, 2017
nashif
nashif requested changes Sep 8, 2017
View reviewed changes
drivers/ethernet/eth_enc28j60.c Outdated
.gpio_pin = 0,
.delay = 0
};
#endif
Copy link
Member

Choose a reason for hiding this comment

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

if we are moving this driver to new API, why are we checking for legacy?

Copy link
Collaborator

@tbursztyka tbursztyka Sep 8, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

The patch makes both SPI API supported, choice being made at built time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nashif the driver tests are still using platforms not supporting the new SPI interface, at the moment supporting both API's gives us a higher test coverage

Copy link
Member

Choose a reason for hiding this comment

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

The patch makes both SPI API supported, choice being made at built time.

we should only support one API, and if a platform still support the old API, it will never be moved if we keep this running in this mode and we will end up with 2 APIs for a long time.

I prefer to have this support 1 single API and accelerate the move to the new API with a deadline.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nashif no problem, I agree with that, but there is still the open question, how to handle the tests then?

@nashif nashif modified the milestones: v1.10, v1.10.0 Oct 3, 2017
@nashif nashif modified the milestones: v1.10.0, v1.11.0 Nov 28, 2017
@locomuco locomuco mentioned this pull request Jan 31, 2018
Merged
@locomuco
Copy link
Contributor Author

locomuco commented Feb 8, 2018

superseeded by #5921

@locomuco locomuco closed this Feb 8, 2018
@locomuco locomuco deleted the enc branch May 5, 2018 08:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nashif nashif nashif requested changes

@tbursztyka tbursztyka tbursztyka left review comments

@jukkar jukkar Awaiting requested review from jukkar

Assignees
No one assigned
Labels
area: Networking
Projects
None yet
Milestone
v1.11.0
Development

Successfully merging this pull request may close these issues.
4 participants
@locomuco @nashif @tbursztyka @jukkar