NRF52 SPI master for new SPI API #5670

vanwinkeljan · 2018-01-14T14:13:18Z

NRF52 SPI Master implementation following new SPI API.
Based on legacy NRF52 SPIM driver.

codecov-io · 2018-01-14T14:35:27Z

Codecov Report

Merging #5670 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #5670   +/-   ##
=======================================
  Coverage   53.28%   53.28%           
=======================================
  Files         424      424           
  Lines       40729    40729           
  Branches     7845     7845           
=======================================
  Hits        21702    21702           
  Misses      15817    15817           
  Partials     3210     3210

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4d72e48...78577b1. Read the comment docs.

tbursztyka

tiny comments

tbursztyka · 2018-01-15T09:45:59Z

drivers/spi/spi_nrf52_master.c

+
+	if ((config->cs != NULL) || (cfg->psel.cs == CS_UNUSED)) {
+		/* Disable HW based CS */
+		spim->PSEL.CSN |= SPIM_PSEL_CSN_CONNECT_Msk;


isn't spi_context_cs_configure() missing here?

tbursztyka · 2018-01-15T09:47:53Z

drivers/spi/spi_nrf52_master.c

+
+	ret = spi_context_wait_for_completion(&data->ctx);
+
+	spi_context_release(&data->ctx, ret);


you could move that line before return ret, so line 225 would be move as line 222. but it's cosmetic

tbursztyka · 2018-01-15T09:49:34Z

drivers/spi/spi_nrf52_master.c

+		      u32_t tx_count, struct spi_buf *rx_bufs, u32_t rx_count,
+		      bool asynchronous, struct k_poll_signal *signal)
+{
+	int ret;


usually, pre-assigned variable declaration come first, so here ret would be the last

tbursztyka · 2018-01-15T10:00:25Z

arch/arm/soc/nordic_nrf5/nrf52/Kconfig.defconfig.nrf52840_QIAA

@@ -14,7 +14,7 @@ config SOC

 config NUM_IRQS
 	int
-	default 46
+	default 48


you commit message does not say why you increase this number (and how it relates to spi actually)

updated commit message to indicate number was increased as per documentation

The lower number of interrupts was causing a build error as the last SPI device has number 47

galak

Should have also introduce device tree support for SPI

galak · 2018-01-23T17:40:55Z

arch/arm/soc/nordic_nrf5/include/nrf5_common.h

@@ -60,6 +60,7 @@
 #define NRF52_IRQ_RTC2_IRQn                    36
 #define NRF52_IRQ_I2S_IRQn                     37
 #define NRF52_IRQ_FPU_IRQn                     38
+#define NRF52_IRQ_SPIM3_IRQn                   47


should come from Device Tree

vanwinkeljan · 2018-02-01T22:32:39Z

updated code after testing

vanwinkeljan · 2018-02-01T23:47:59Z

@galak added initial attempt for dts

vanwinkeljan · 2018-02-06T09:27:41Z

Addresses NRF52 SPI master part of #4713

carlescufi · 2018-02-20T17:17:25Z

@vanwinkeljan I know @anangl is preparing to submit a driver based on nrfx which now lives in ext/hal/nordic. There's also a rework of the SPI API that @tbursztyka is leading and that will affect this. I think we should join efforts here and provide an nrfx-powered driver that complies with the new API. @anangl will be back next week from holiday and can discuss this further.

mbolivar · 2018-02-21T18:23:22Z

I'm trying to get this to build for nrf52_pca10040 by adding the following to hello_world's prj.conf:

# CONFIG_SPI_LEGACY_API is not set
CONFIG_SPI=y
CONFIG_SPI_0=y
CONFIG_SPI_NRF5=y
CONFIG_SPI0_NRF52_MASTER=y

I get a lot of warnings and errors: https://gist.github.com/mbolivar/13d9da829fd5d0d6501c604fc0380d29

Am I missing something?

vanwinkeljan · 2018-02-21T21:32:41Z

@mbolivar I will have a look at it.

For now I know there is a problem for SPI0 if I switch to SPI1 I do not seen the warnings.
But even then I get compile errors for nrf52_pca10040 due to missing declarations (I tested with nrf52840_pca10056).

One further remark is that you will also need to add a DTS overlay (nrf52_pca10040.overlay) to the hello_world directory that enables the SPI port and declares which pins it will use.
e.g.
&spi1 {
status = "ok";
clk-pin = <3>;
mosi-pin = <28>;
miso-pin = <29>;
};

Increased the number of interrupts from 46 to 48 according to nrf52840 documentation. Signed-off-by: Jan Van Winkel <[email protected]>

Added NRF52 SPI master using new SPI API. Signed-off-by: Jan Van Winkel <[email protected]>

Added DTS support for NRF52 master SPI driver Signed-off-by: Jan Van Winkel <[email protected]>

vanwinkeljan · 2018-02-22T07:58:05Z

Corrected SPI Kconfig to take in account that DTS can be used for SPI0
Take in account that there is only a psel CSN for nrf52840

@mbolivar this should fix your build issues

mbolivar · 2018-03-21T17:13:15Z

@vanwinkeljan looks like the PR from @anangl (#6458) got taken in #5921 -- earlier @carlescufi mentioned this code as a potential place to work together. Any comments on where you want this PR to go? Sorry that I haven't gotten around to re-testing, as the patches in 6458 did the trick for me.

vanwinkeljan · 2018-03-21T17:53:28Z

@mbolivar I noticed @anangl PR and skimmed through and had no remarks.
I will close this PR

vanwinkeljan requested a review from tbursztyka as a code owner January 14, 2018 14:13

carlescufi requested a review from anangl January 14, 2018 18:26

tbursztyka requested changes Jan 15, 2018

View reviewed changes

galak requested changes Jan 23, 2018

View reviewed changes

galak reviewed Jan 23, 2018

View reviewed changes

vanwinkeljan mentioned this pull request Jan 31, 2018

SPI API cleanup and update #5921

Merged

vanwinkeljan force-pushed the nrf52_spi_master_pull_req branch from de9d0f8 to 0fadbd5 Compare February 1, 2018 22:23

vanwinkeljan changed the title ~~[RFC] NRF52 SPI master for new SPI API~~ NRF52 SPI master for new SPI API Feb 6, 2018

vanwinkeljan added 3 commits February 22, 2018 08:50

arm: nrf52840: Corrected number of interrupts

ffc3fb2

Increased the number of interrupts from 46 to 48 according to nrf52840 documentation. Signed-off-by: Jan Van Winkel <[email protected]>

drivers: spi: New SPI API NRF52 Master

f72f8ba

Added NRF52 SPI master using new SPI API. Signed-off-by: Jan Van Winkel <[email protected]>

dts: spi: Added DTS support for NRF52 master

78577b1

Added DTS support for NRF52 master SPI driver Signed-off-by: Jan Van Winkel <[email protected]>

vanwinkeljan force-pushed the nrf52_spi_master_pull_req branch from 1eba990 to 78577b1 Compare February 22, 2018 07:54

vanwinkeljan closed this Mar 21, 2018

vanwinkeljan deleted the nrf52_spi_master_pull_req branch March 24, 2018 20:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NRF52 SPI master for new SPI API #5670

NRF52 SPI master for new SPI API #5670

vanwinkeljan commented Jan 14, 2018 •

edited

Loading

codecov-io commented Jan 14, 2018 •

edited

Loading

tbursztyka left a comment

tbursztyka Jan 15, 2018

tbursztyka Jan 15, 2018

tbursztyka Jan 15, 2018

tbursztyka Jan 15, 2018

vanwinkeljan Feb 1, 2018

galak left a comment

galak Jan 23, 2018

vanwinkeljan commented Feb 1, 2018

vanwinkeljan commented Feb 1, 2018

vanwinkeljan commented Feb 6, 2018

carlescufi commented Feb 20, 2018

mbolivar commented Feb 21, 2018

vanwinkeljan commented Feb 21, 2018

vanwinkeljan commented Feb 22, 2018

mbolivar commented Mar 21, 2018

vanwinkeljan commented Mar 21, 2018


		ret = spi_context_wait_for_completion(&data->ctx);

		spi_context_release(&data->ctx, ret);

NRF52 SPI master for new SPI API #5670

NRF52 SPI master for new SPI API #5670

Conversation

vanwinkeljan commented Jan 14, 2018 • edited Loading

codecov-io commented Jan 14, 2018 • edited Loading

Codecov Report

tbursztyka left a comment

Choose a reason for hiding this comment

tbursztyka Jan 15, 2018

Choose a reason for hiding this comment

tbursztyka Jan 15, 2018

Choose a reason for hiding this comment

tbursztyka Jan 15, 2018

Choose a reason for hiding this comment

tbursztyka Jan 15, 2018

Choose a reason for hiding this comment

vanwinkeljan Feb 1, 2018

Choose a reason for hiding this comment

galak left a comment

Choose a reason for hiding this comment

galak Jan 23, 2018

Choose a reason for hiding this comment

vanwinkeljan commented Feb 1, 2018

vanwinkeljan commented Feb 1, 2018

vanwinkeljan commented Feb 6, 2018

carlescufi commented Feb 20, 2018

mbolivar commented Feb 21, 2018

vanwinkeljan commented Feb 21, 2018

vanwinkeljan commented Feb 22, 2018

mbolivar commented Mar 21, 2018

vanwinkeljan commented Mar 21, 2018

vanwinkeljan commented Jan 14, 2018 •

edited

Loading

codecov-io commented Jan 14, 2018 •

edited

Loading