Pass ssel PinName to spi_init() for hardware assisted SSEL control.

[jeremybrodt]

Some SPI master implementations control the slave select in hardware. Passing the pin name on to the C HAL for configuration.

[0xc0170]

Have you looked at how spi_init() for targets and SPISlave are implemented? The last parameter defines if it's master or slave, not the ssel pin.

This will require more changes, than just passing down the ssel. One option can be to expand the spi_init() by another parameter, which defines slave/spi, this needs to be implemented for all current targets.

[jeremybrodt]

Thanks for your feedback. I think you may be confusing spi_init() and spi_format(). Here are the current prototypes.

void spi_init(spi_t *obj, PinName mosi, PinName miso, PinName sclk, PinName ssel);
void spi_format (spi_t *obj, int bits, int mode, int slave);

The last parameter to spi_init() is already ssel. It is just currently not used in SPI.cpp.

[0xc0170]

I am not. The confusion is there, as clearly the argument's name is different than it's use.
Look at SPISlave and SPImaster ctors, how they invoke spi_init and then what's in the spi_init(), a snippet from lpc1768:

    // set default format and frequency
    if (ssel == NC) {
        spi_format(obj, 8, 0, 0);  // 8 bits, mode 0, master
    } else {
        spi_format(obj, 8, 0, 1);  // 8 bits, mode 0, slave
    }

It is currently used, to pass the info if it's slave or master,therefore that NC in SPI ctor. Not certain what platforms you have use to test your change. We need to change HAL, to correct it there first - to remove the above from all HAL spi_init functions, as format is called from SPI/SPISlave ctors anyway.

[jeremybrodt]

I see the problem. That's unfortunate that other HAL are expecting the value to be NC, and are using it for some meaning other than what it is named for. I agree with you about how it should be fixed. How does a change a that scale get implemented? I am new to this process.

[jeremybrodt]

I just noticed that the default value of that parameter in SPI.h is 'NC.' Won't this provide the required backwards compatibility with the LPC implementations?

[Sissors]

That will mean by default it will indeed still function. However if you pass the pin you want to use for hardware SSEL control, if will still make it an SPI slave device automatically. In addition to this the SPI class would need extra arguments to control the SSEL: you need to be able to tell it if it should raise SSEL after transmitting the word you wrote, or if it should keep it low. Then some targets can only have a single SSEL per SPI, some can have multiple. And what if you have several SPI objects on a single port, where one uses hardware SSEL, others DigitalOut SSEL?

So it would require extra arguments on SPI functions, so changing the API, and probably all SPI implementations have to be modified. If you say that the current implementation of that init function is bad, then I fully agree with you. But modifying it will be a huge undertaking. Probably easiest to start with modifications in all target specific SPI files that start using a new init function and where they ignore SSEL in master mode. And then later they can one at a time be modified to use SSEL. I don't know how large the advantage is compared to DigitalOut though. And then you need to use different code to write to SPI if you have hardware SSEL or if you have no hardware SSEL. Or ideally if there is no hardware SSEL it automatically creates a DigitalOut. Still alltogether a huge undertaking.

[0xc0170]

I am going to close this as it would break the current targets implementation. To enable hw chip select in the C++ API, the HAL should to be changed first. You can create an issue for spi HW chip select, to track this improvement. Thanks for understanding

[0xc0170]

Created a new issue, to correct HAL spi init()