Hello!
Some questions and suggestions regarding the SPI interface.
1. What is the expected behavior of
hal_spi_txrx()?---------------------------------------------------
>From hal_spi.h:
```
/**
...
* @param spi_num SPI interface to use
* @param txbuf Pointer to buffer where values to transmit are stored.
* @param rxbuf Pointer to buffer to store values received from peer.
* @param cnt Number of 8-bit or 16-bit values to be transferred.
*
* @return int 0 on success, non-zero error code on failure.
*/
int hal_spi_txrx(int spi_num, void *txbuf, void *rxbuf, int cnt);
```
- Can `txbuf` and/or `rxbuf` be NULL? I suspect this is handled in some nrf
ports but it do not seem to work with stm32 ports. It is not clear from the
comment whatever this is a bug in the stm32 port or expected behavior.
(The comments seems like a good place to put this sort of info)
- why is `txbuf` param not const? Is a implementation allowed to modify this
data? I suspect not!? Adding const will probably not be seen in the assembler
output but makes the expected behavior clear. It also allows higher
abstraction layers to use const without ugly casting.
2. Error codes from hal functions?
----------------------------------
I do not know if this have been discussed before, but is (non-zero) error codes
from hal functions supposed to be same for all ports? I have noticed that
ST HAL error codes is sometimes converted to -1, but this seems like throwing
away useful debug information (a "magic" non-zero error code in the log is
better
then nothing).
3. Suggestion: remove hal_spi_tx_val()--------------------------------------
This function returns 0xffff on error which seems like a ambiguous result
when 16 bit spi used. It is also reduntant to hal_spi_txrx().
Backward compatible function:
```
// deprecated
static inline uint16_t hal_spi_tx_val(int spi_num, uint16_t txval)
{
uint16_t rxval = 0;
return hal_spi_txrx(spi_num, &val, rxd, 1)) ? 0xFFFF : rxval;
}
```
4. Suggestion: remove
hal_spi_data_mode_breakout()--------------------------------------------------
Redundant function as the information is already encoded as follows:
```
#define HAL_SPI_MODE_CPHA_MSK (0x1)
#define HAL_SPI_MODE_CPOL_MSK (0x2)
/** SPI mode 0. CPOL=0, CPHA=0 */
#define HAL_SPI_MODE0 (0)
/** SPI mode 1. CPOL=0, CPHA=1 */
#define HAL_SPI_MODE1 (HAL_SPI_MODE_CPHA_MSK) // 1
/** SPI mode 2. CPOL=1, CPHA=0 */
#define HAL_SPI_MODE2 (HAL_SPI_MODE_CPOL_MSK) // 2
/** SPI mode 3, CPOL=1, CPHA=1 */
#define HAL_SPI_MODE3 (HAL_SPI_MODE_CPOL_MSK | HAL_SPI_MODE_CPHA_MSK) // 3
```i.e. all HAL_SPI_MODEn values same as before.
Backward compatible function (and usage example):
```
// deprecated
static inline int hal_spi_data_mode_breakout(uint8_t mode,
int *out_cpol, int *out_cpha)
{
*out_cpol = mode & HAL_SPI_MODE_CPOL_MSK;
*out_cpha = mode & HAL_SPI_MODE_CPHA_MSK;
return 0;
}
```
Best Regars // Simon F.
