i2s: Add an nrfx-based implementation for Nordic ICs #19436

adamkondraciuk · 2019-09-27T13:24:26Z

adamkondraciuk · 2019-09-27T13:26:23Z

carlescufi · 2019-09-27T13:28:08Z

@adamkondraciuk thanks for the PR. Could you perhaps take the .pdf and convert it to a combination of .rst and .png that we could place somewhere in doc? Otherwise we just leave it out of the PR and that's fine too.

zephyrbot · 2019-09-27T13:34:50Z

All checks are passing now.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

carlescufi · 2019-09-30T09:45:58Z

@adamkondraciuk thanks for the PR. Could you perhaps take the .pdf and convert it to a combination of .rst and .png that we could place somewhere in doc? Otherwise we just leave it out of the PR and that's fine too.

On second thought, we don't really have anywhere to place this kind of documentation today, and it would probably be very difficult to keep it updated, so let's not include it in the Pull Request

boards/arm/nrf52840_pca10056/nrf52840_pca10056.dts

drivers/i2s/i2s_nrfx.c

anangl

A couple of initial comments. More to come.

drivers/i2s/Kconfig.nrfx

drivers/i2s/i2s_nrfx.h

drivers/i2s/i2s_nrfx.c

drivers/i2s/i2s_nrfx.h

drivers/i2s/i2s_nrfx.c

koffes · 2019-10-10T06:47:37Z

CODEOWNERS

@@ -176,6 +176,8 @@
 /drivers/i2c/i2c_rv32m1_lpi2c*            @henrikbrixandersen
 /drivers/i2c/*sam0*                       @Sizurka
 /drivers/i2c/i2c_dw*                      @gnuless
+/drivers/i2s/Kconfig.nrfx                 @adamkondraciuk


Is there some example code you can add as well? Having a sample that e.g. sends incremental numbers over I2S would greatly assist developers in getting started.

Is there some example code you can add as well? Having a sample that e.g. sends incremental numbers over I2S would greatly assist developers in getting started.

I'll push it as another PR after driver review

drivers/i2s/i2s_nrfx.c

anangl · 2019-12-13T12:49:04Z

drivers/i2s/i2s_nrfx.c

+{									      \
+	IRQ_CONNECT(DT_NORDIC_NRF_I2S_I2S_##idx##_IRQ_0,		      \
+		DT_NORDIC_NRF_I2S_I2S_##idx##_IRQ_0_PRIORITY,		      \
+		isr, DEVICE_GET(i2s_##idx), 0);				      \


Use:

Suggested change

isr, DEVICE_GET(i2s_##idx), 0); \

nrfx_isr, nrfx_i2s_irq_handler, 0); \

and remove the isr function.

anangl · 2019-12-13T14:23:18Z

drivers/i2s/i2s_nrfx.c

+ *   next to be transferred (content of 'TXD.PTR' register).
+ *   Status parameter informs about handler execution reason:
+ *   - if 1 - next buffer is needed
+ *   - if 0 - transfer is finishing


This is not guaranteed. nrfx_i2s API does not specify it so.

anangl · 2019-12-13T14:40:05Z

drivers/i2s/i2s_nrfx.c

+	struct channel_str *ch_rx = &i2s->channel_rx;
+	int ret;
+
+	if (p_released->p_rx_buffer != NULL && next_buffers_needed(status)) {


p_released itself may also be NULL, see nrfx_i2s.h.

anangl · 2019-12-16T09:40:00Z

drivers/i2s/i2s_nrfx.c

+ * - allocates new rx buffer for next transfer purpose
+ * Status parameter informs about handler execution reason:
+ *   - if 1 - next buffer is needed
+ *   - if 0 - transfer is finishing


This is not guaranteed. nrfx_i2s API does not specify it so.

anangl · 2019-12-16T09:40:37Z

drivers/i2s/i2s_nrfx.c

+		}
+		return;
+	}
+	if (p_released->p_tx_buffer != NULL) {


p_released itself may also be NULL, see nrfx_i2s.h.

anangl · 2019-12-16T09:50:29Z

drivers/i2s/i2s_nrfx.c

+	if (p_released->p_rx_buffer != NULL && next_buffers_needed(status)) {
+		/* Content of received buffer is valuable.
+		 * If 'EVENT_STOPPED' generated 'then next_buffers_needed()'
+		 * returns false - function 'channel_rx_store_data()' won't


You need to use some different logic here. next_buffers_needed() returns false when the event handler was called without the driver's request of next buffers, what does not necessarily mean that the STOPPED event has been generated. nrfx_i2s API does not specify it this way, so you cannot rely on it.

anangl · 2019-12-16T09:54:27Z

drivers/i2s/i2s_nrfx.c

+		    next_buffers_needed(status)) {
+			/* when interface needs to be restarted and
+			 * last event was 'EVENT_STOPPED' then we don't
+			 * free this buffer - it will be used after


As in channel_rx_callback, you cannot use next_buffers_needed to check if the transfer was stopped.

anangl · 2019-12-16T10:11:49Z

drivers/i2s/i2s_nrfx.c

+		} else {
+			if (p_released->p_rx_buffer) {
+				ret = channel_rx_store_data(i2s,
+					(u32_t **)&p_released->p_rx_buffer);


This part of code needs to be described. For instance, why is this data stored only when next buffers were not requested by the nrfx_i2s driver?

anangl · 2019-12-16T10:24:37Z

drivers/i2s/i2s_nrfx.c

+				break;
+			default:
+				channel_change_state(ch_tx, I2S_STATE_ERROR);
+				return;


This part of code needs to be described as well, and perhaps reworked. Currently, it is too hard to understand what is actually going on here. For instance, what happens with mem_block here (when the error is signaled)? When will it get freed?

anangl · 2019-12-16T10:31:19Z

drivers/i2s/i2s_nrfx.c

+}
+
+static void channel_tx_mem_clear(struct i2s_nrfx_data *i2s,
+				 void const *first_block)


This function is called only from two places, and only once it is called with first_block not being NULL. Why not free this first block right there, get rid of this parameter, and thus seriously simplify whole this function?

Changed board definitions to use I2S driver Signed-off-by: Adam Kondraciuk <adam.kondraciuk@nordicsemi.no>

Translation layer to make I2S drivers work with Zephyr API. This shim is provided only for master-stereo transmission Signed-off-by: Adam Kondraciuk <adam.kondraciuk@nordicsemi.no>

mnkp · 2019-12-30T00:48:44Z

Since i2s interface and API are quite complex this driver should be tested with tests/drivers/i2s/ tests. Unfortunately, the tests assume that the driver supports I2S_OPT_LOOPBACK flag. They need to be modified to support also hardware loopback mode.

koffes · 2020-02-21T09:38:33Z

The driver has a function called interface_error_service(struct i2s_nrfx_data *i2s, const char * const err_msg).
It would be great if this instead of taking a string could take some form of int .
This function may then again be used to alert the application in some way. We are in a position where we need to get updates of TX underruns and RX overruns.

anangl · 2021-04-13T14:34:49Z

This PR has been superseded by #34251.

adamkondraciuk requested review from carlescufi, ioannisg and lemrey as code owners September 27, 2019 13:24

carlescufi requested review from anangl, kl-cruz and pizi-nordic September 27, 2019 13:26

zephyrbot added area: Devicetree area: I2S area: Boards labels Sep 27, 2019

carlescufi added area: Drivers platform: nRF Nordic nRFx labels Sep 27, 2019

adamkondraciuk force-pushed the i2s-nrfx branch from 3a542cd to f6319be Compare September 27, 2019 14:15

adamkondraciuk force-pushed the i2s-nrfx branch from f6319be to 1c4c8f8 Compare September 30, 2019 11:13

pizi-nordic previously requested changes Sep 30, 2019

View reviewed changes

boards/arm/nrf52840_pca10056/nrf52840_pca10056.dts Show resolved Hide resolved

drivers/i2s/i2s_nrfx.c Outdated Show resolved Hide resolved

adamkondraciuk force-pushed the i2s-nrfx branch 2 times, most recently from 5d6ef85 to f78b470 Compare October 1, 2019 11:24

anangl requested changes Oct 2, 2019

View reviewed changes

adamkondraciuk force-pushed the i2s-nrfx branch from f78b470 to ccd909f Compare October 4, 2019 09:14

anangl reviewed Oct 7, 2019

View reviewed changes

anangl reviewed Oct 8, 2019

View reviewed changes

koffes reviewed Oct 10, 2019

View reviewed changes

adamkondraciuk force-pushed the i2s-nrfx branch from ccd909f to b5f54d3 Compare October 11, 2019 09:50

anangl reviewed Oct 14, 2019

View reviewed changes

drivers/i2s/i2s_nrfx.c Outdated Show resolved Hide resolved

adamkondraciuk force-pushed the i2s-nrfx branch from b5f54d3 to ea0b580 Compare October 18, 2019 12:10

koffes mentioned this pull request Oct 28, 2019

drivers: i2s: Added API to get driver state #20180

Closed

adamkondraciuk force-pushed the i2s-nrfx branch 2 times, most recently from 7dbb34b to a435051 Compare November 22, 2019 10:13

anangl reviewed Nov 25, 2019

View reviewed changes

anangl mentioned this pull request Dec 10, 2019

nrfx_i2s: Introduce NRFX_I2S_STATUS_TRANSFER_STOPPED flag zephyrproject-rtos/hal_nordic#16

Merged

carlescufi changed the title ~~I2s nrfx~~ i2s: Add an nrfx-based implementation for Nordic ICs Dec 11, 2019

adamkondraciuk force-pushed the i2s-nrfx branch from a435051 to 93b388d Compare December 12, 2019 15:41

zephyrbot added the area: Modules label Dec 12, 2019

anangl reviewed Dec 16, 2019

View reviewed changes

boards: nrf: Add I2S support

7fce56b

Changed board definitions to use I2S driver Signed-off-by: Adam Kondraciuk <adam.kondraciuk@nordicsemi.no>

adamkondraciuk force-pushed the i2s-nrfx branch from 93b388d to 197e63e Compare December 17, 2019 08:35

qdrivers: i2s: nrfx: Add shim for nrfx I2S master driver

bc88dbc

Translation layer to make I2S drivers work with Zephyr API. This shim is provided only for master-stereo transmission Signed-off-by: Adam Kondraciuk <adam.kondraciuk@nordicsemi.no>

adamkondraciuk force-pushed the i2s-nrfx branch from 197e63e to bc88dbc Compare December 19, 2019 10:25

github-actions bot added has-conflicts Issue/PR has conflicts with another issue/PR area: Device Tree labels Jun 29, 2020

galak removed the area: Device Tree label Jul 1, 2020

github-actions bot added has-conflicts Issue/PR has conflicts with another issue/PR and removed has-conflicts Issue/PR has conflicts with another issue/PR labels Jul 1, 2020

nashif added the Stale label Sep 4, 2020

github-actions bot closed this Sep 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

i2s: Add an nrfx-based implementation for Nordic ICs #19436

i2s: Add an nrfx-based implementation for Nordic ICs #19436

adamkondraciuk commented Sep 27, 2019

adamkondraciuk commented Sep 27, 2019

carlescufi commented Sep 27, 2019

zephyrbot commented Sep 27, 2019 •

edited

carlescufi commented Sep 30, 2019

anangl left a comment •

edited

koffes Oct 10, 2019

adamkondraciuk Oct 11, 2019

anangl Dec 13, 2019

anangl Dec 13, 2019

anangl Dec 13, 2019

anangl Dec 16, 2019

anangl Dec 16, 2019

anangl Dec 16, 2019

anangl Dec 16, 2019

anangl Dec 16, 2019

anangl Dec 16, 2019

anangl Dec 16, 2019

mnkp commented Dec 30, 2019

koffes commented Feb 21, 2020

anangl commented Apr 13, 2021

	isr, DEVICE_GET(i2s_##idx), 0); \
	nrfx_isr, nrfx_i2s_irq_handler, 0); \

i2s: Add an nrfx-based implementation for Nordic ICs #19436

i2s: Add an nrfx-based implementation for Nordic ICs #19436

Conversation

adamkondraciuk commented Sep 27, 2019

adamkondraciuk commented Sep 27, 2019

carlescufi commented Sep 27, 2019

zephyrbot commented Sep 27, 2019 • edited

carlescufi commented Sep 30, 2019

anangl left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnkp commented Dec 30, 2019

koffes commented Feb 21, 2020

anangl commented Apr 13, 2021

zephyrbot commented Sep 27, 2019 •

edited

anangl left a comment •

edited