add SPI migration guide #766
Conversation
| @@ -0,0 +1,133 @@ | |||
| <h2 id="spi-port">Serial Peripheral Interface (SPI)</h2> | |||
There was a problem hiding this comment.
@ithinuel : This is a HAL API doc & not a user facing API doc, correct? If so, could you please add a line in the introduction that this is only for the HAL..thanks.
There was a problem hiding this comment.
This is in the target porting guide. All chapter that live here relate to the hal layer.
The other guides don't have any warning about that.
Should this be added to all other guides too ?
There was a problem hiding this comment.
Thanks @ithinuel . In that case, the warning is not required. Thank you.
docs/porting/target/spi.md
Outdated
| - `spi_free()` disables the peripheral clock. | ||
| - `spi_format()` sets : | ||
| - the number of bits per symbol | ||
| - the mode : |
There was a problem hiding this comment.
@AnotherButler : Seems to be a md formatting issue here.. mode 0 isn't being treated as a bullet point..
docs/porting/target/spi.md
Outdated
|
|
||
| - `spi_get_module()` returns the `SPIName` unique identifier to the peripheral associated to this SPI channel. | ||
| - `spi_get_capabilities()` fills the given `spi_capabilities_t` instance | ||
| - `spi_get_capabilities()` should consider the `ssel` pin when evaluation the `support_slave_mode` capability. |
There was a problem hiding this comment.
SSEL pin may have different names for each target (SSEL, NCS etc).. how will this be handled?
There was a problem hiding this comment.
The actual name of the value does not matter much as what's considered by the spi_get_capabilities is the argument that's passed to it.
| - `spi_format()` updates the configuration of the peripheral except the baud rate generator. | ||
| - `spi_frequency()` sets the frequency to use during the transfer. | ||
| - `spi_frequency()` returns the actual frequency that will be used. | ||
| - `spi_frequency()` updates the baud rate generator leaving other configurations unchanged. |
There was a problem hiding this comment.
Why format() should leave baud rate generator un-touched? And the frequency() should only configure the baud rate generator? Can we have format() setting everything and frequency() change "only" the baud rate gen along with the other 2 functions above?
There was a problem hiding this comment.
Because the responsibility of spi_format is only to configure the format of the frames. Changing the baudrate generator in spi_format could be considered as a side effect as it overlaps with the dedicated method spi_frequency.
The choice of having two functions was made during the RFC process because the format is very unlikely to change during the lifetime of a spi instance while the frequency is likely to be raised after the first few exchanges with the slave peripheral.
docs/porting/target/spi.md
Outdated
| A target must also define these elements: | ||
|
|
||
| - `#define SPI_COUNT (xxxxxU)`. | ||
| The number of SPI peripheral available on the device. |
There was a problem hiding this comment.
Might be helpful to provide the filename where this should be added..
There was a problem hiding this comment.
I have declared in PeripheralNames.h in the reference implementation but is it THE best place ?
I looked sensible to me as the driver includes hal/spi_api.h that in turn includes PeripheralNames.h.
What do you think about it ?
There was a problem hiding this comment.
@ithinuel : Yes, I was referring to PeripheralNames.h . Can you maybe add a note in this line to add the number of peripherals there?
|
@c1728p9 Is this ready for my review? |
|
@AnotherButler yeah, this is ready |
Copy edit file, mostly for agreement, formatting and spelling.
Add space to fix formatting.
|
Query: It looks like the Doxygen links don't work - Does that mean this is the first time we're using this feature branch? Do I need to create it? |
|
ping @c1728p9 |
|
@AnotherButler I think it is. Yes, please create it |
|
|
||
| You can read more about the test cases: | ||
|
|
||
| [](http://os.mbed.com/docs/development/feature-hal-spec-spi-doxy/group__hal__spi__tests.html) |
There was a problem hiding this comment.
I've generated the branch, and the SPI class link earlier in the doc works. However, this one doesn't. Could you please look into it?
Add SPI HAL API migration guide