Hi Will,

On Mon, Sep 19, 2016 at 06:07:55PM -0700, will sanfilippo wrote:
> The new HAL SPI API is shown below. Highlights of the changes are:

These changes look great.  I just have a comment and a question:

> /**
>  * Initialize the SPI, given by spi_num.
>  *
>  * @param spi_num The number of the SPI to initialize
>  * @param cfg HW/MCU specific configuration,
>  *            passed to the underlying implementation, providing extra
>  *            configuration.
>  * @param spi_type SPI type (master or slave)
>  *
>  * @return int 0 on success, non-zero error code on failure.
>  */
> int hal_spi_init(int spi_num, void *cfg, uint8_t spi_type);

My comment here might apply to the HAL in general.  I wonder if there is
any value in including an init function in the HAL abstraction.
Initialization is hardware-specific, as evidenced by the void*
parameter, so wouldn't it be better to just have the caller invoke the
specific function that applies to the MCU?  Unless there is a need to
initialize a SPI without needing to know what MCU you are running on, I
would say the init function should be left out of the HAL, as there is
nothing being abstracted away.

> /**
>  * Sets the default value transferred by the slave. Not valid for master
>  *
>  * @param spi_num SPI interface to use
>  *
>  * @return int 0 on success, non-zero error code on failure.
>  */
> int hal_spi_slave_set_def_tx_val(int spi_num, uint16_t val);

Does this function specify the transmitted character until the next
transceive operation, or does it only apply for txrx operations with a
null rx buffer?  Put another way, if a slave executes the following
sequence of operations:

1. Initialize SPI.
2. Enable SPI.
3. Set default character to 0x99.

And then nothing else, will the slave respond to the master with 0x99


Reply via email to