Skip to content
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

added spi_get_baudrate() + some consistency changes #395

Merged
merged 9 commits into from
Jun 1, 2021
Merged

added spi_get_baudrate() + some consistency changes #395

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ecc002c
added spi_get_baudrate() + some consistency changes
Reneg973 May 11, 2021
f385948
fixed docu
Reneg973 May 11, 2021
ae375ab
Merge branch 'develop' into SPI-Helpers
Reneg973 May 11, 2021
b43ac69
Update spi.h
Reneg973 May 12, 2021
272bd55
Merge branch 'SPI-Helpers' of https://github.com/Reneg973/pico-sdk in…
Reneg973 May 12, 2021
05224d2
another misleading docu removed
Reneg973 May 12, 2021
5a043a2
improved const correctness
Reneg973 May 13, 2021
b0e2b1e
fixed kitchen_sink issues
Reneg973 May 14, 2021
5b2f444
reverted size_t return value to int
Reneg973 May 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 34 additions & 17 deletions src/rp2_common/hardware_spi/include/hardware/spi.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,18 +132,28 @@ void spi_deinit(spi_inst_t *spi);
*/
uint spi_set_baudrate(spi_inst_t *spi, uint baudrate);

/*! \brief Get SPI baudrate
* \ingroup hardware_spi
*
* Get SPI baudrate which was set by \see spi_set_baudrate
*
* \param spi SPI instance specifier, either \ref spi0 or \ref spi1
* \return The actual baudrate set
*/
uint spi_get_baudrate(const spi_inst_t *spi);

/*! \brief Convert SPI instance to hardware instance number
* \ingroup hardware_spi
*
* \param spi SPI instance
* \return Number of SPI, 0 or 1.
*/
static inline uint spi_get_index(spi_inst_t *spi) {
static inline uint spi_get_index(const spi_inst_t *spi) {
invalid_params_if(SPI, spi != spi0 && spi != spi1);
return spi == spi1 ? 1 : 0;
}

static inline spi_hw_t *spi_get_hw(spi_inst_t *spi) {
static inline spi_hw_t *spi_get_hw(const spi_inst_t *spi) {
Copy link
Contributor

Choose a reason for hiding this comment

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

don't think this should be const; sure the method itself doesn't write thru it, but it sure does cast it to a mutable pointer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I propose an alternative, add an additional function:
static inline const spi_hw_t *spi_get_hw_const(const spi_inst_t *spi) {

Needs some more changes. Agreed?

Copy link
Contributor

Choose a reason for hiding this comment

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

we don't do this for any hardware; not least there may be registers for which reading has side effects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Your decision, both ways are possible. Anyway my experience is that const correctness can expose possible issues in code.

E.g. check function hw_is_claimed(), the function name says it just checks, but in reality it claims, so it does more than expected. One of the rules I try to follow - no surprises. And if hw_is_claimed() modifies something another function (hw_claim_or_assert) is made for, IMO this is a surprise

Copy link
Contributor

Choose a reason for hiding this comment

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

ha - yes that is a bug! I'm all for const pointers to any data struct, i was just on the fence in the h/w pointer case. Anyway, we can't do this as it requires a cast away of const... as you say we could add a spi_get_hw_const which I'm not whoilly against, but I don't think is necessary.

Copy link
Contributor Author

@Reneg973 Reneg973 May 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

right, that's ok here, it's just a simple function where const correctness is not of any meaning. I'll remove it - but then it must be removed for every const correct function which uses it... compiler thingy

Copy link
Contributor Author

@Reneg973 Reneg973 May 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I made a proposal with commit 5a043a2. Please check whether it's ok or needs a revert

spi_get_index(spi); // check it is a hw spi
return (spi_hw_t *)spi;
}
Expand Down Expand Up @@ -197,27 +207,34 @@ static inline void spi_set_slave(spi_inst_t *spi, bool slave) {
* \ingroup hardware_spi
*
* \param spi SPI instance specifier, either \ref spi0 or \ref spi1
* \return 0 if no space is available to write. Non-zero if a write is possible
* \return false if no space is available to write. True if a write is possible
*
* \note Although the controllers each have a 8 deep TX FIFO, the current HW implementation can only return 0 or 1
* rather than the space available.
*/
static inline size_t spi_is_writable(spi_inst_t *spi) {
// PL022 doesn't expose levels directly, so return values are only 0 or 1
return (spi_get_hw(spi)->sr & SPI_SSPSR_TNF_BITS) >> SPI_SSPSR_TNF_LSB;
static inline bool spi_is_writable(const spi_inst_t *spi) {
kilograham marked this conversation as resolved.
Show resolved Hide resolved
return (spi_get_hw(spi)->sr & SPI_SSPSR_TNF_BITS);
}
Reneg973 marked this conversation as resolved.
Show resolved Hide resolved

/*! \brief Check whether a read can be done on SPI device
* \ingroup hardware_spi
*
* \param spi SPI instance specifier, either \ref spi0 or \ref spi1
* \return Non-zero if a read is possible i.e. data is present
* \return true if a read is possible i.e. data is present
*
* \note Although the controllers each have a 8 deep RX FIFO, the current HW implementation can only return 0 or 1
Reneg973 marked this conversation as resolved.
Show resolved Hide resolved
* rather than the data available.
*/
static inline size_t spi_is_readable(spi_inst_t *spi) {
return (spi_get_hw(spi)->sr & SPI_SSPSR_RNE_BITS) >> SPI_SSPSR_RNE_LSB;
static inline bool spi_is_readable(const spi_inst_t *spi) {
return (spi_get_hw(spi)->sr & SPI_SSPSR_RNE_BITS);
Reneg973 marked this conversation as resolved.
Show resolved Hide resolved
}

/*! \brief Check whether SPI is busy
* \ingroup hardware_spi
*
* \param spi SPI instance specifier, either \ref spi0 or \ref spi1
* \return true if SPI is busy
*/
static inline bool spi_is_busy(const spi_inst_t *spi) {
return (spi_get_hw(spi)->sr & SPI_SSPSR_BSY_BITS);
kilograham marked this conversation as resolved.
Show resolved Hide resolved
}

/*! \brief Write/Read to/from an SPI device
Expand All @@ -232,7 +249,7 @@ static inline size_t spi_is_readable(spi_inst_t *spi) {
* \param len Length of BOTH buffers
* \return Number of bytes written/read
*/
int spi_write_read_blocking(spi_inst_t *spi, const uint8_t *src, uint8_t *dst, size_t len);
size_t spi_write_read_blocking(spi_inst_t *spi, const uint8_t *src, uint8_t *dst, size_t len);

/*! \brief Write to an SPI device, blocking
* \ingroup hardware_spi
Expand All @@ -245,7 +262,7 @@ int spi_write_read_blocking(spi_inst_t *spi, const uint8_t *src, uint8_t *dst, s
* \param len Length of \p src
* \return Number of bytes written/read
*/
int spi_write_blocking(spi_inst_t *spi, const uint8_t *src, size_t len);
size_t spi_write_blocking(spi_inst_t *spi, const uint8_t *src, size_t len);

/*! \brief Read from an SPI device
* \ingroup hardware_spi
Expand All @@ -262,7 +279,7 @@ int spi_write_blocking(spi_inst_t *spi, const uint8_t *src, size_t len);
* \param len Length of buffer \p dst
* \return Number of bytes written/read
*/
int spi_read_blocking(spi_inst_t *spi, uint8_t repeated_tx_data, uint8_t *dst, size_t len);
size_t spi_read_blocking(spi_inst_t *spi, uint8_t repeated_tx_data, uint8_t *dst, size_t len);

// ----------------------------------------------------------------------------
// SPI-specific operations and aliases
Expand All @@ -283,7 +300,7 @@ int spi_read_blocking(spi_inst_t *spi, uint8_t repeated_tx_data, uint8_t *dst, s
* \param len Length of BOTH buffers in halfwords
* \return Number of halfwords written/read
*/
int spi_write16_read16_blocking(spi_inst_t *spi, const uint16_t *src, uint16_t *dst, size_t len);
size_t spi_write16_read16_blocking(spi_inst_t *spi, const uint16_t *src, uint16_t *dst, size_t len);

/*! \brief Write to an SPI device
* \ingroup hardware_spi
Expand All @@ -298,7 +315,7 @@ int spi_write16_read16_blocking(spi_inst_t *spi, const uint16_t *src, uint16_t *
* \param len Length of buffers
* \return Number of halfwords written/read
*/
int spi_write16_blocking(spi_inst_t *spi, const uint16_t *src, size_t len);
size_t spi_write16_blocking(spi_inst_t *spi, const uint16_t *src, size_t len);

/*! \brief Read from an SPI device
* \ingroup hardware_spi
Expand All @@ -317,7 +334,7 @@ int spi_write16_blocking(spi_inst_t *spi, const uint16_t *src, size_t len);
* \param len Length of buffer \p dst in halfwords
* \return Number of halfwords written/read
*/
int spi_read16_blocking(spi_inst_t *spi, uint16_t repeated_tx_data, uint16_t *dst, size_t len);
size_t spi_read16_blocking(spi_inst_t *spi, uint16_t repeated_tx_data, uint16_t *dst, size_t len);

#ifdef __cplusplus
}
Expand Down
31 changes: 18 additions & 13 deletions src/rp2_common/hardware_spi/spi.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ uint spi_init(spi_inst_t *spi, uint baudrate) {
spi_set_format(spi, 8, SPI_CPOL_0, SPI_CPHA_0, SPI_MSB_FIRST);
// Always enable DREQ signals -- harmless if DMA is not listening
hw_set_bits(&spi_get_hw(spi)->dmacr, SPI_SSPDMACR_TXDMAE_BITS | SPI_SSPDMACR_RXDMAE_BITS);
spi_set_format(spi, 8, SPI_CPOL_0, SPI_CPHA_0, SPI_MSB_FIRST);
kilograham marked this conversation as resolved.
Show resolved Hide resolved

// Finally enable the SPI
hw_set_bits(&spi_get_hw(spi)->cr1, SPI_SSPCR1_SSE_BITS);
Expand Down Expand Up @@ -66,9 +65,15 @@ uint spi_set_baudrate(spi_inst_t *spi, uint baudrate) {
return freq_in / (prescale * postdiv);
}

uint spi_get_baudrate(const spi_inst_t *spi) {
uint prescale = spi_get_hw(spi)->cpsr;
uint postdiv = ((spi_get_hw(spi)->cr0 & SPI_SSPCR0_SCR_BITS) >> SPI_SSPCR0_SCR_LSB) + 1;
return clock_get_hz(clk_peri) / (prescale * postdiv);
}

// Write len bytes from src to SPI. Simultaneously read len bytes from SPI to dst.
// Note this function is guaranteed to exit in a known amount of time (bits sent * time per bit)
int __not_in_flash_func(spi_write_read_blocking)(spi_inst_t *spi, const uint8_t *src, uint8_t *dst, size_t len) {
size_t __not_in_flash_func(spi_write_read_blocking)(spi_inst_t *spi, const uint8_t *src, uint8_t *dst, size_t len) {
invalid_params_if(SPI, 0 > (int)len);

// Never have more transfers in flight than will fit into the RX FIFO,
Expand All @@ -87,11 +92,11 @@ int __not_in_flash_func(spi_write_read_blocking)(spi_inst_t *spi, const uint8_t
}
}

return (int)len;
return len;
}

// Write len bytes directly from src to the SPI, and discard any data received back
int __not_in_flash_func(spi_write_blocking)(spi_inst_t *spi, const uint8_t *src, size_t len) {
size_t __not_in_flash_func(spi_write_blocking)(spi_inst_t *spi, const uint8_t *src, size_t len) {
invalid_params_if(SPI, 0 > (int)len);
// Write to TX FIFO whilst ignoring RX, then clean up afterward. When RX
// is full, PL022 inhibits RX pushes, and sets a sticky flag on
Expand All @@ -113,14 +118,14 @@ int __not_in_flash_func(spi_write_blocking)(spi_inst_t *spi, const uint8_t *src,
// Don't leave overrun flag set
spi_get_hw(spi)->icr = SPI_SSPICR_RORIC_BITS;

return (int)len;
return len;
}

// Read len bytes directly from the SPI to dst.
// repeated_tx_data is output repeatedly on SO as data is read in from SI.
// Generally this can be 0, but some devices require a specific value here,
// e.g. SD cards expect 0xff
int __not_in_flash_func(spi_read_blocking)(spi_inst_t *spi, uint8_t repeated_tx_data, uint8_t *dst, size_t len) {
size_t __not_in_flash_func(spi_read_blocking)(spi_inst_t *spi, uint8_t repeated_tx_data, uint8_t *dst, size_t len) {
invalid_params_if(SPI, 0 > (int)len);
const size_t fifo_depth = 8;
size_t rx_remaining = len, tx_remaining = len;
Expand All @@ -136,11 +141,11 @@ int __not_in_flash_func(spi_read_blocking)(spi_inst_t *spi, uint8_t repeated_tx_
}
}

return (int)len;
return len;
}

// Write len halfwords from src to SPI. Simultaneously read len halfwords from SPI to dst.
int __not_in_flash_func(spi_write16_read16_blocking)(spi_inst_t *spi, const uint16_t *src, uint16_t *dst, size_t len) {
size_t __not_in_flash_func(spi_write16_read16_blocking)(spi_inst_t *spi, const uint16_t *src, uint16_t *dst, size_t len) {
invalid_params_if(SPI, 0 > (int)len);
// Never have more transfers in flight than will fit into the RX FIFO,
// else FIFO will overflow if this code is heavily interrupted.
Expand All @@ -158,11 +163,11 @@ int __not_in_flash_func(spi_write16_read16_blocking)(spi_inst_t *spi, const uint
}
}

return (int)len;
return len;
}

// Write len bytes directly from src to the SPI, and discard any data received back
int __not_in_flash_func(spi_write16_blocking)(spi_inst_t *spi, const uint16_t *src, size_t len) {
size_t __not_in_flash_func(spi_write16_blocking)(spi_inst_t *spi, const uint16_t *src, size_t len) {
invalid_params_if(SPI, 0 > (int)len);
// Deliberately overflow FIFO, then clean up afterward, to minimise amount
// of APB polling required per halfword
Expand All @@ -182,12 +187,12 @@ int __not_in_flash_func(spi_write16_blocking)(spi_inst_t *spi, const uint16_t *s
// Don't leave overrun flag set
spi_get_hw(spi)->icr = SPI_SSPICR_RORIC_BITS;

return (int)len;
return len;
}

// Read len halfwords directly from the SPI to dst.
// repeated_tx_data is output repeatedly on SO as data is read in from SI.
int __not_in_flash_func(spi_read16_blocking)(spi_inst_t *spi, uint16_t repeated_tx_data, uint16_t *dst, size_t len) {
size_t __not_in_flash_func(spi_read16_blocking)(spi_inst_t *spi, uint16_t repeated_tx_data, uint16_t *dst, size_t len) {
invalid_params_if(SPI, 0 > (int)len);
const size_t fifo_depth = 8;
size_t rx_remaining = len, tx_remaining = len;
Expand All @@ -203,5 +208,5 @@ int __not_in_flash_func(spi_read16_blocking)(spi_inst_t *spi, uint16_t repeated_
}
}

return (int)len;
return len;
}