xmos / lib_sdram

SDRAM library
Other
1 stars 5 forks source link

API has superfluous arguments #16

Open ed-xmos opened 7 years ago

ed-xmos commented 7 years ago

In addition to the ports, the sdram server API is currently passed:

Finally, it makes sense to wrap up both of the port list (5 ports) and SDRAM config arguments into typedef-ed structures.

samchesney commented 7 years ago

We might want to reconsider wrapping ports into a structure as it will introduce an inconsistent style across our libraries (see IP-CS116), the rational being:

Structures work well if passed around to multiple places. In this case they are only passed in at the top level so adding a structure is an extra level of indirection. Without this port allocations in applications are clearer. This system also allows easy sharing of clock arguments with other components (if marked const).

If the ports are bound together in some way with a default initializer (e.g. OTP or XAB libraries) then a structure may still be appropriate.

andrewstanfordjason commented 7 years ago

The reason everything was exposed was to be inline with the coding guidelines at the time. I prefer all things like that in a struct

ed-xmos commented 7 years ago

I can see the arguments for both approaches. I think the SDRAM parameters should definitely be in a type-deffed struct. That way, you can just pass in an initialised struct for a part number like "sdram_IS42S16400D". We can provide a few examples of common parts. For the ports, it's probably clearer for the user to have less indirection but more convenient for us to have a struct when you have cases like slice kit where there are multiple slots that are supported (as well as multiple boards). So you could have an initialised struct such as ports_slicekit_X200_triangle_tile_0 or similar. I propose going for struct for the sdram parameters and resource IDs for the ports/clockblk unless anyone has strong objections, and keep the permutations of the examples to a minimum to keep it cleaner.