[GreatFET] ssp -> spi in libgreat

[Kamil Rogozinski]

On 30/9/19 9:15 am, Garret Kelly wrote:
> Hi GreatFET gang,
> I was looking at adding SPI support to libgreat, and was thinking
> about how SSPs aren't SPI.  Is the thinking that there should be an
> API that looks something like:
> 
> platform_ssp_t *ssp = platform_ssp1_init();
> spi_t *spi = platform_ssp_to_spi(ssp);
> spi_write(spi, data, len);
> 
> And spi_t would have function pointers for write or transact and
> spi_write would just be an inline call through that function pointer.
> The platform_ssp_to_spi would build a spi_t that could unwrap the
> spi_t back to platform_ssp_t so it could do the actual write.
> 
> But it does mean there would still need to be platform-specific init
> code that knows how to 'get' an spi_t for a specific class.  Maybe I'm
> thinking about this wrong and spi_init should behave like:
> 
> spi_t *spi = spi_init(PLATFORM_SSP0);
> spi_write(spi, data, len);
> 
> And spi_init would defer to platform_spi_init, which would know how to
> turn specific platform_spi_handle_t (or some better name) into the
> corresponding spi_t that wrapped the SSP it was referencing.
> 
> Having written that out I like the look of the approach because it
> looks more concise, and has a very uniform pattern:
> 
> spi_init(PLATFORM_SPI_SPI); // bare SPI controller on lpc43xx (not SSP)
> spi_init(PLATFORM_SPI_SSP0); // SPI on SSP0, etc.
> 
> But it does mean that there's going to be a big switch somewhere that
> dispatches the choice of which static spi_t implementation struct to
> return probably.  I was thinking that one of the downsides of that
> approach is that now platform_ssp_t isn't available when you want to
> do more complex things, but I think that platform_spi_init could just
> as easily be written in terms of the first approach:
> spi_init(platform_spi_handle_t) calls platform_spi_init calls
> platform_sspX_init, wraps and returns it with platform_ssp_to_spi,
> etc.
> 
> The only remaining nit I have with the second approach is that
> spi_init is not platform code, so making it take a platform type feels
> gross.  In the first approach there will be per-platform code for
> particular classes, but no layering violations like in the second
> approach.
> 
> Therefore, I think I'm concluding that the first approach is my
> favourite.  Before I go write this, does anyone have any different
> conclusions or additional thoughts?
> 
> Sorry for rambling!
> 
> Thanks!
> Garret
> _______________________________________________
> GreatFET mailing list
> GreatFET at greatscottgadgets.com
> https://pairlist3.pair.net/mailman/listinfo/greatfet
> 

Hi Garret,

I've only just received my GreatFET and haven't made any contributions 
yet, so please take my reply as a comment or opinion only.

I tend to agree with your assessment and prefer the first approach of 
keeping platform specific knowledge and types out of the user-facing 
API. Passing a handle to spi_init() is one way to invoke the platform 
specific initialisation. Otherwise, you could also include function 
pointers to the platform specific init/read/write functions in your 
spi_t struct type and pass that into spi_init(), much like you're 
already doing with spi_write(). This will give you a consistent API.

As I mentioned, I haven't dug into the libgreat code base yet, but happy 
to review contributions and provide feedback as I familiarise myself 
with it.

Thanks,
Kamil