From 4c06dca15ce5e152544fd05ecdf4a7eb2402bd74 Mon Sep 17 00:00:00 2001 From: Jeroen Domburg Date: Thu, 13 Apr 2017 11:14:35 +0800 Subject: [PATCH] SPI: Small fixes according to MR comments --- components/driver/include/driver/spi_common.h | 16 ++++---- components/driver/include/driver/spi_slave.h | 32 ++++++++-------- components/driver/spi_common.c | 10 +---- components/driver/spi_master.c | 3 -- components/driver/spi_slave.c | 11 ++---- components/driver/test/test_spi_master.c | 38 +++++++++---------- docs/api/peripherals/spi_slave.rst | 8 ++-- examples/peripherals/spi_slave/README.md | 18 ++++++++- .../peripherals/spi_slave/receiver/Makefile | 2 +- .../peripherals/spi_slave/sender/Makefile | 2 +- tools/unit-test-app/sdkconfig | 2 +- 11 files changed, 73 insertions(+), 69 deletions(-) diff --git a/components/driver/include/driver/spi_common.h b/components/driver/include/driver/spi_common.h index e410c75b8f..c3aaa8da45 100644 --- a/components/driver/include/driver/spi_common.h +++ b/components/driver/include/driver/spi_common.h @@ -1,4 +1,4 @@ -// Copyright 2010-2016 Espressif Systems (Shanghai) PTE LTD +// Copyright 2010-2017 Espressif Systems (Shanghai) PTE LTD // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -78,9 +78,9 @@ bool spicommon_periph_claim(spi_host_device_t host); bool spicommon_periph_free(spi_host_device_t host); -#define SPICOMMON_BUSFLAG_SLAVE 0 ///< Initialize I/O in slave mode -#define SPICOMMON_BUSFLAG_MASTER 1 ///< Initialize I/O in master mode -#define SPICOMMON_BUSFLAG_QUAD 2 ///< Also initialize WP/HD pins, if specified +#define SPICOMMON_BUSFLAG_SLAVE 0 ///< Initialize I/O in slave mode +#define SPICOMMON_BUSFLAG_MASTER (1<<0) ///< Initialize I/O in master mode +#define SPICOMMON_BUSFLAG_QUAD (1<<1) ///< Also initialize WP/HD pins, if specified /** * @brief Connect a SPI peripheral to GPIO pins @@ -93,13 +93,13 @@ bool spicommon_periph_free(spi_host_device_t host); * @param bus_config Pointer to a spi_bus_config struct detailing the GPIO pins * @param dma_chan DMA-channel (1 or 2) to use, or 0 for no DMA. * @param flags Combination of SPICOMMON_BUSFLAG_* flags - * @param is_native A value of 'true' will be written to this address if the GPIOs can be + * @param[out] is_native A value of 'true' will be written to this address if the GPIOs can be * routed using the IO_mux, 'false' if the GPIO matrix is used. * @return * - ESP_ERR_INVALID_ARG if parameter is invalid * - ESP_OK on success */ -esp_err_t spicommon_bus_initialize_io(spi_host_device_t host, spi_bus_config_t *bus_config, int dma_chan, int flags, bool *is_native); +esp_err_t spicommon_bus_initialize_io(spi_host_device_t host, const spi_bus_config_t *bus_config, int dma_chan, int flags, bool *is_native); /** * @brief Free the IO used by a SPI peripheral @@ -170,7 +170,7 @@ int spicommon_irqsource_for_host(spi_host_device_t host); /** - * @note V0 and V1 of the ESP32 silicon has a bug where in some (well-known) cases a SPI DMA channel will get confused. This can be remedied + * @note In some (well-defined) cases in the ESP32 (at least rev v.0 and v.1), a SPI DMA channel will get confused. This can be remedied * by resetting the SPI DMA hardware in case this happens. Unfortunately, the reset knob used for thsi will reset _both_ DMA channels, and * as such can only done safely when both DMA channels are idle. These functions coordinate this. * @@ -190,7 +190,7 @@ typedef void(*dmaworkaround_cb_t)(void *arg); /** * @brief Request a reset for a certain DMA channel * - * @param host The SPI host + * @param dmachan DMA channel associated with the SPI host that needs a reset * @param cb Callback to call in case DMA channel cannot be reset immediately * @param arg Argument to the callback * diff --git a/components/driver/include/driver/spi_slave.h b/components/driver/include/driver/spi_slave.h index c27809bc8b..9045c29288 100644 --- a/components/driver/include/driver/spi_slave.h +++ b/components/driver/include/driver/spi_slave.h @@ -1,4 +1,4 @@ -// Copyright 2010-2016 Espressif Systems (Shanghai) PTE LTD +// Copyright 2010-2017 Espressif Systems (Shanghai) PTE LTD // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -31,7 +31,6 @@ extern "C" #define SPI_SLAVE_TXBIT_LSBFIRST (1<<0) ///< Transmit command/address/data LSB first instead of the default MSB first #define SPI_SLAVE_RXBIT_LSBFIRST (1<<1) ///< Receive data LSB first instead of the default MSB first #define SPI_SLAVE_BIT_LSBFIRST (SPI_TXBIT_LSBFIRST|SPI_RXBIT_LSBFIRST); ///< Transmit and receive LSB first -#define SPI_SLAVE_POSITIVE_CS (1<<3) ///< Make CS positive during a transaction instead of negative typedef struct spi_slave_transaction_t spi_slave_transaction_t; @@ -41,9 +40,9 @@ typedef void(*slave_transaction_cb_t)(spi_slave_transaction_t *trans); * @brief This is a configuration for a SPI host acting as a slave device. */ typedef struct { - int spics_io_num; ///< CS GPIO pin for this device, or -1 if not used - uint32_t flags; ///< Bitwise OR of SPI_DEVICE_* flags - int queue_size; ///< Transaction queue size. This sets how many transactions can be 'in the air' (queued using spi_device_queue_trans but not yet finished using spi_device_get_trans_result) at the same time + int spics_io_num; ///< CS GPIO pin for this device + uint32_t flags; ///< Bitwise OR of SPI_SLAVE_* flags + int queue_size; ///< Transaction queue size. This sets how many transactions can be 'in the air' (queued using spi_slave_queue_trans but not yet finished using spi_slave_get_trans_result) at the same time uint8_t mode; ///< SPI mode (0-3) slave_transaction_cb_t post_setup_cb; ///< Callback called after the SPI registers are loaded with new data slave_transaction_cb_t post_trans_cb; ///< Callback called after a transaction is done @@ -56,6 +55,7 @@ struct spi_slave_transaction_t { size_t length; ///< Total data length, in bits const void *tx_buffer; ///< Pointer to transmit buffer, or NULL for no MOSI phase void *rx_buffer; ///< Pointer to receive buffer, or NULL for no MISO phase + void *user; ///< User-defined variable. Can be used to store eg transaction ID. }; /** @@ -75,7 +75,7 @@ struct spi_slave_transaction_t { * - ESP_ERR_NO_MEM if out of memory * - ESP_OK on success */ -esp_err_t spi_slave_initialize(spi_host_device_t host, spi_bus_config_t *bus_config, spi_slave_interface_config_t *slave_config, int dma_chan); +esp_err_t spi_slave_initialize(spi_host_device_t host, const spi_bus_config_t *bus_config, const spi_slave_interface_config_t *slave_config, int dma_chan); /** * @brief Free a SPI bus claimed as a SPI slave interface @@ -97,26 +97,27 @@ esp_err_t spi_slave_free(spi_host_device_t host); * unhandled transactions before it and the master initiates a SPI transaction by pulling down CS and sending out * clock signals. * - * @param handle Device handle obtained using spi_host_add_dev - * @param trans_desc Description of transaction to execute + * @param host SPI peripheral that is acting as a slave + * @param trans_desc Description of transaction to execute. Not const because we may want to write status back + * into the transaction description. * @param ticks_to_wait Ticks to wait until there's room in the queue; use portMAX_DELAY to * never time out. * @return * - ESP_ERR_INVALID_ARG if parameter is invalid * - ESP_OK on success */ -esp_err_t spi_slave_queue_trans(spi_host_device_t host, spi_slave_transaction_t *trans_desc, TickType_t ticks_to_wait); +esp_err_t spi_slave_queue_trans(spi_host_device_t host, const spi_slave_transaction_t *trans_desc, TickType_t ticks_to_wait); /** * @brief Get the result of a SPI transaction queued earlier * * This routine will wait until a transaction to the given device (queued earlier with - * spi_device_queue_trans) has succesfully completed. It will then return the description of the + * spi_slave_queue_trans) has succesfully completed. It will then return the description of the * completed transaction so software can inspect the result and e.g. free the memory or * re-use the buffers. * - * @param handle Device handle obtained using spi_host_add_dev + * @param host SPI peripheral to that is acting as a slave * @param trans_desc Pointer to variable able to contain a pointer to the description of the * transaction that is executed * @param ticks_to_wait Ticks to wait until there's a returned item; use portMAX_DELAY to never time @@ -131,13 +132,14 @@ esp_err_t spi_slave_get_trans_result(spi_host_device_t host, spi_slave_transacti /** * @brief Do a SPI transaction * - * Essentially does the same as spi_device_queue_trans followed by spi_device_get_trans_result. Do + * Essentially does the same as spi_slave_queue_trans followed by spi_slave_get_trans_result. Do * not use this when there is still a transaction queued that hasn't been finalized - * using spi_device_get_trans_result. + * using spi_slave_get_trans_result. * - * @param handle Device handle obtained using spi_host_add_dev + * @param host SPI peripheral to that is acting as a slave * @param trans_desc Pointer to variable able to contain a pointer to the description of the - * transaction that is executed + * transaction that is executed. Not const because we may want to write status back + * into the transaction description. * @param ticks_to_wait Ticks to wait until there's a returned item; use portMAX_DELAY to never time * out. * @return diff --git a/components/driver/spi_common.c b/components/driver/spi_common.c index 4b098b8309..0287e66a50 100644 --- a/components/driver/spi_common.c +++ b/components/driver/spi_common.c @@ -19,7 +19,6 @@ #include "soc/spi_reg.h" #include "soc/dport_reg.h" #include "soc/spi_struct.h" -#include "soc/rtc_cntl_reg.h" #include "rom/ets_sys.h" #include "esp_types.h" #include "esp_attr.h" @@ -27,16 +26,9 @@ #include "esp_intr_alloc.h" #include "esp_log.h" #include "esp_err.h" -#include "freertos/FreeRTOS.h" -#include "freertos/semphr.h" -#include "freertos/xtensa_api.h" -#include "freertos/task.h" -#include "freertos/ringbuf.h" #include "soc/soc.h" #include "soc/dport_reg.h" -#include "soc/uart_struct.h" #include "rom/lldesc.h" -#include "driver/uart.h" #include "driver/gpio.h" #include "driver/periph_ctrl.h" #include "esp_heap_alloc_caps.h" @@ -189,7 +181,7 @@ Do the common stuff to hook up a SPI host to a bus defined by a bunch of GPIO pi bus config struct and it'll set up the GPIO matrix and enable the device. It will set is_native to 1 if the bus config can be done using the IOMUX instead of using the GPIO matrix. */ -esp_err_t spicommon_bus_initialize_io(spi_host_device_t host, spi_bus_config_t *bus_config, int dma_chan, int flags, bool *is_native) +esp_err_t spicommon_bus_initialize_io(spi_host_device_t host, const spi_bus_config_t *bus_config, int dma_chan, int flags, bool *is_native) { bool native=true; bool is_master=(flags&SPICOMMON_BUSFLAG_MASTER)?true:false; diff --git a/components/driver/spi_master.c b/components/driver/spi_master.c index 06d29081e1..28e964787e 100644 --- a/components/driver/spi_master.c +++ b/components/driver/spi_master.c @@ -40,7 +40,6 @@ queue and re-enabling the interrupt will trigger the interrupt again, which can #include "soc/spi_reg.h" #include "soc/dport_reg.h" #include "soc/spi_struct.h" -#include "soc/rtc_cntl_reg.h" #include "rom/ets_sys.h" #include "esp_types.h" #include "esp_attr.h" @@ -55,9 +54,7 @@ queue and re-enabling the interrupt will trigger the interrupt again, which can #include "freertos/ringbuf.h" #include "soc/soc.h" #include "soc/dport_reg.h" -#include "soc/uart_struct.h" #include "rom/lldesc.h" -#include "driver/uart.h" #include "driver/gpio.h" #include "driver/periph_ctrl.h" #include "esp_heap_alloc_caps.h" diff --git a/components/driver/spi_slave.c b/components/driver/spi_slave.c index f321799908..d5c9fed7c0 100644 --- a/components/driver/spi_slave.c +++ b/components/driver/spi_slave.c @@ -19,7 +19,6 @@ #include "soc/spi_reg.h" #include "soc/dport_reg.h" #include "soc/spi_struct.h" -#include "soc/rtc_cntl_reg.h" #include "rom/ets_sys.h" #include "esp_types.h" #include "esp_attr.h" @@ -34,9 +33,7 @@ #include "freertos/ringbuf.h" #include "soc/soc.h" #include "soc/dport_reg.h" -#include "soc/uart_struct.h" #include "rom/lldesc.h" -#include "driver/uart.h" #include "driver/gpio.h" #include "driver/periph_ctrl.h" #include "esp_heap_alloc_caps.h" @@ -48,7 +45,7 @@ static const char *SPI_TAG = "spi_slave"; return (ret_val); \ } -#define VALID_HOST(x) (host>SPI_HOST && host<=VSPI_HOST) +#define VALID_HOST(x) (x>SPI_HOST && x<=VSPI_HOST) typedef struct { spi_slave_interface_config_t cfg; @@ -68,7 +65,7 @@ static spi_slave_t *spihost[3]; static void IRAM_ATTR spi_intr(void *arg); -esp_err_t spi_slave_initialize(spi_host_device_t host, spi_bus_config_t *bus_config, spi_slave_interface_config_t *slave_config, int dma_chan) +esp_err_t spi_slave_initialize(spi_host_device_t host, const spi_bus_config_t *bus_config, const spi_slave_interface_config_t *slave_config, int dma_chan) { bool native, claimed; //We only support HSPI/VSPI, period. @@ -200,7 +197,7 @@ esp_err_t spi_slave_free(spi_host_device_t host) } -esp_err_t spi_slave_queue_trans(spi_host_device_t host, spi_slave_transaction_t *trans_desc, TickType_t ticks_to_wait) +esp_err_t spi_slave_queue_trans(spi_host_device_t host, const spi_slave_transaction_t *trans_desc, TickType_t ticks_to_wait) { BaseType_t r; SPI_CHECK(VALID_HOST(host), "invalid host", ESP_ERR_INVALID_ARG); @@ -368,7 +365,7 @@ static void IRAM_ATTR spi_intr(void *arg) host->hw->user.usr_miso_highpart=0; host->hw->user.usr_mosi_highpart=0; if (trans->tx_buffer) { - uint32_t *data=host->cur_trans->tx_buffer; + const uint32_t *data=host->cur_trans->tx_buffer; for (int x=0; xlength; x+=32) { uint32_t word; memcpy(&word, &data[x/32], 4); diff --git a/components/driver/test/test_spi_master.c b/components/driver/test/test_spi_master.c index 9a55f69990..665f432481 100644 --- a/components/driver/test/test_spi_master.c +++ b/components/driver/test/test_spi_master.c @@ -150,14 +150,14 @@ static void spi_test(spi_device_handle_t handle, int num_bytes) { int from=x-16; if (from<0) from=0; printf("Error at %d! Sent vs recved: (starting from %d)\n" , x, from); - for (int i=0; i<32; i++) { - if (i+from