Comments inline


> On Sep 19, 2016, at 6:40 PM, Christopher Collins <ccoll...@apache.org> wrote:
> 
> 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.
> 

Well, you do bring up a point, but I feel there is some benefit in having a 
consistent hal_xxx_init function across HALs. If only so that developers know 
they must implement and call this function at startup.

This API does allow for one small abstration though, the spi type (master or 
slave). Yeah, that is nit-picky :-)

>> /**
>> * 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
> characters?
> 
> Thanks,
> Chris

This description definitely needs improving and I think this API needs a bit of 
thought as well. To answer your question, yes, that was part of the intent. It 
was also intended to cover these cases:

1) A NULL txbuf was used as the parameter to hal_spi_txrx_noblock() and the SPI 
is configured as a slave.
2) The master sends more data than the value supplied in ‘cnt’. 

Admittedly, I think this presumes some HW support. It may not be possible to 
guarantee what happens in these cases. There may be some SPI devices that 
simply re-transmit the last byte loaded or do something completely different.

Thanks for the comments!


 


Reply via email to