drivers/spi: updated SPI driver interface

[haukepetersen]

• made datastructures overridable by CPU implementations
• made some functions void
• updated doxygen
• adapted SPI test
• adapted SPI implementations
• fixed some bugs in spi transfer functions

The main idea behind this API change is to adapt it to a slightly changed paradigm regarding error handling: The concept is to throw errors on initialization, but to not check all parameters everytime afterwards anymore. This leads to more compact and efficient code while not taking much away in terms of error checking.

On top of this, most implementations are implemented in a way, that there return values were quite useless anyway...

This PR is just the first step in 'modernizing' the existing SPI implementations...

NOTE: This needs some testing on all affected platforms once we agree on the changes...

[kaspar030]

... interface

[haukepetersen]

changed interface slightly, so that transfer_byte and transfer_reg return the received byte. Also adapted (most) drivers.

[jnohlgard]

you can remove the (int) cast since both len and i are size_t

[haukepetersen]

true, will fix.

[jnohlgard]

I like this improvement.

Could we somehow prepare the SPI interface for hardware CS support too while we are doing API changes?

I don't really know what the best API model would be here, the CS parameter must be somehow conveyed to the SPI driver, either through a function argument or through some global variable (e.g. a driver struct or similar)

[haukepetersen]

Lets take it step by step I would say. This improvement is basically the base for further work (as not it is possible, to define a 'whatever-you-like-struct' as SPI device identifier on a per-board basis.

With the CS I see a different difficulty: this is how we handle CS inside device drivers. I would hate to see constructs involving ifdefs on weather to call gpio_set/clear manually or not...

[haukepetersen]

Hm, wait: I might have an idea: We could introduce another parameter for the transfer functions:

void spi_transfer_bytes(spi_t dev, gpio_t cs, ...);

Now the spi driver can deicide itself, if it just ignores the given parameter (and uses internal HW CS lines instead). Even both is possible, as you could implement your CPUs spi driver in a way, that it will use some HW CS pin in case the parameter 'GPIO_UNDEF' is given, and use SW CS.

Using an additional parameter has the advantage, that the SPI driver does not need to keep state. Because keeping say an instance struct for each device that is run on a SPI bus, it would lead to redundant data that can be saved this way. What do you say?

[jnohlgard]

@haukepetersen

With the CS I see a different difficulty: this is how we handle CS inside device drivers. I would hate to see constructs involving ifdefs on weather to call gpio_set/clear manually or not...

I completely agree, which is why I would like to open the discussion on how to handle CS lines within the device drivers.

Hm, wait: I might have an idea: We could introduce another parameter for the transfer functions:

void spi_transfer_bytes(spi_t dev, gpio_t cs, ...);

Now the spi driver can deicide itself, if it just ignores the given parameter (and uses internal HW CS lines instead). Even both is possible, as you could implement your CPUs spi driver in a way, that it will use some HW CS pin in case the parameter 'GPIO_UNDEF' is given, and use SW CS.

Using an additional parameter has the advantage, that the SPI driver does not need to keep state. Because keeping say an instance struct for each device that is run on a SPI bus, it would lead to redundant data that can be saved this way. What do you say?

This is similar to what we have been using in Contiki for a while. You need to add one more parameter though, void spi_transfer_bytes(spi_t dev, gpio_t cs, bool continue, ...); to tell the spi_transfer_bytes function to keep asserting the CS line after the transfer is completed. Some transactions are made up of multiple transfers while the CS line is held (e.g. write address+read data bytes).

[jnohlgard]

If we update the SPI API this way it will also be possible to implement DMA transfers for SPI transfers on platforms where that is supported (e.g. Kinetis)

[haukepetersen]

About the parameter continue: I think we might as well find a way around it, maybe by giving gpio_t some defined value to point this out?

For DMA we wouldn't have to change anything, this is already perfectly feasible with todays API (just nobody found the time to implement it for any platform, yet). But if this kind of change would make it easier - even better!

[haukepetersen]

I think I had one idea more: How about we define a new type for the additional spi transfer function parameter, maybe spi_cs_t or something. This type MUST be defined in a way, that gpio_t can be cast onto it, but it further allows for additional values. So say the GPIO's are identified by positive integers (GPIO_0 := 0; GPIO_1 :=1, ..., GPIO_UNDEF := -1), you could add values as SPI_CS_HW0 := -2, SPI_CS_HW1 := -3, ..., SPI_CS_NOCS := INT_MIN` and so on. These values should be of course defined on a CPU basis with whatever values that are the most efficient on a platform. So in a driver device descriptor, you'd have:

struct {
    ...
    spi_cs_t cs;
    ...
} dev_t;

which you can initialize by either

dev_init(..., (spi_cs_t)GPIO(x,y), ...);

or

dev_init(..., SPI_CS_HW(x), ...);

What do you think?

[jnohlgard]

@haukepetersen I don't see a way around the cont parameter unless we redefine the cs parameter to be a bitfield where we can have a "continue" bit and then the GPIO number.

Your idea for the CS parameter and using special values for HW CS is fine by me, that gives us the flexibility to use both GPIO software CS, and HW CS. But it still requires a continue parameter to tell the SPI bus driver whether to de-assert the CS pin after the transaction or not.

[PeterKietzmann]

@gebart is there a special sensor you are thinking about when proposing the cont parameter? I still have problems to see the need of that parameter besides the spi_transfer_regs function which sends an address and data during one call, or just resetting the CS line after such a command.

[jnohlgard]

@PeterKietzmann e.g. https://github.com/RIOT-OS/RIOT/blob/master/drivers/ng_at86rf2xx/ng_at86rf2xx_internal.c#L68

[jnohlgard]

it is possible to find many more examples if you look for it, both in implemented drivers and future device drivers.

edit: e.g. https://github.com/RIOT-OS/RIOT/blob/master/drivers/nvram_spi/nvram-spi.c#L194

[PeterKietzmann]

@gebart thanks! It's been a while writing such a driver. My first idea was to use the transfer_regs function instead of calling transfer_re and transfer_bytes. Anyway, this could lead to ugly results (if that's at least possible). Guess I have to think about it a bit.

[jnohlgard]

@PeterKietzmann Some devices reset their internal communication state (e.g an address pointer to an SRAM location), when the CS is de-asserted, so it will have to be handled in a single _regs call, which will lead to the implementation of some temporary transfer buffers on the MCU side for the at86rf2xx SRAM write case, and I am not sure it is possible to implement SRAM read with the current API in a single call.

@PeterKietzmann Do you agree that a continue parameter is a good addition to the API after seeing these examples?

[PeterKietzmann]

I agree that it is a good parameter when introducing the HW CS capability. Also I see that HW CS seems to be needed as this topic occurs periodically. I just don't like the idea of having this parameter in the API but using the external CS line without the need of it. That's why I have to think about it a while or become friends with that idea :-)

[jnohlgard]

Read Hauke's reply above on moving the gpio pin to a function argument of
the transfer function. In my view this is the best solution, even for
software CS. This means simpler device drivers, regardless of what platform
we are running on.
On Jun 29, 2015 11:28 AM, "Peter Kietzmann" notifications@github.com
wrote:

I agree that it is a good parameter when introducing the HW CS capability.
Also I see that HW CS seems to be needed as this topic occurs periodically.
I just don't like the idea of having this parameter in the API but using
the external CS line without the need of it. That's why I have to think
about it a while or become friends with that idea :-)

—
Reply to this email directly or view it on GitHub
#3216 (comment).

[PeterKietzmann]

Ok, that would force us to use a cont parameter in either ways, right?

[jnohlgard]

Yes
On Jun 29, 2015 11:48 AM, "Peter Kietzmann" notifications@github.com
wrote:

Ok, that would force us to use a cont parameter in either ways, right?

—
Reply to this email directly or view it on GitHub
#3216 (comment).

[haukepetersen]

rebased, though the PR needs some love...

[haukepetersen]

did some more changes (everything pretty much WIP):

• changed the interface to use uint8_t instead of char
• remodeled the test application to demonstrate the usage of the interface

[jnohlgard]

needs rebase

[haukepetersen]

not only that :-) Can't promise when I get to it, hopefully soon!

[OlegHahm]

Probably not before Monday, right?

[haukepetersen]

definitely not...

[miri64]

How about Tuesday ;-)?

[haukepetersen]

Some Tuesday. It is still on my list, but it seems there is more important stuff coming up all the time...

[kYc0o]

any news on this PR?

[haukepetersen]

still trying to rework the concept for shared buses. I think I found a suitable solution, but need to investigate some more first. Expect to have something for the next release at the latest...

[jnohlgard]

should we close this in favour of #4780?

[haukepetersen]

Makes sense. I left this open for some readon, but I can't remember it anymore, so I can't have been important :-)