Attention is currently required from: Stefan Reinauer.

Patch set 1:Code-Review +1

View Change

11 comments:

• File tf530_spi.c:

  • Patch Set #1, Line 17: * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA

    I don't think we need this paragraph

  • Patch Set #1, Line 20: #if CONFIG_TF530_SPI == 1

    Is this really necessary? I know other files have these guards, but I don't think they're actually useful.

  • Patch Set #1, Line 62: static struct ConfigDev *cd = NULL;

    There's an ongoing effort to remove global state from programmers. See changes to `digilent_spi.c` in CB:54012 and CB:54067 as an example of what should be changed here.

  • Patch Set #1, Line 64:

    #define TF53x_CTRL_CS0 1
    #define TF53x_CTRL_CS1 2
    #define TF53x_CTRL_BUSY 4
    #define TF53x_CTRL_CS2 8
    

    Looks like these are bits within a register. Why not rewrite them as shifts?

     #define TF53x_CTRL_CS0  (1 << 0)
     #define TF53x_CTRL_CS1  (1 << 1)
     #define TF53x_CTRL_BUSY (1 << 2)
     #define TF53x_CTRL_CS2  (1 << 3)

  • Patch Set #1, Line 71:

    uint8_t busy = 0;
    
    	while (busy == 0) {
    		busy = port->ctrl & TF53x_CTRL_BUSY;
    	}
    

    nit: I'd remove the `busy` variable, the name is confusing (it's active-low):

     do {} while ((port->ctrl & TF53x_CTRL_BUSY) == 0);

    Same below.

  • Patch Set #1, Line 106: return 0;

    Don't we need to release `struct ConfigDev *cd` or similar?

  • Patch Set #1, Line 119: unsigned int

    Support for for-loop initial declarations was introduced with C99. Some old compilers that do not use C99 mode by default may complain about it. It's not a big deal in this particular case because this programmer is tied to a particular OS, but it's something to keep in mind.

  • Patch Set #1, Line 125: uint8_t volatile

    nit: `volatile uint8_t`

    Also, would this work?

     (volatile uint8_t)spi_recv_byte(port);

  • Patch Set #1, Line 149:

    
    	struct Library *ExpansionBase = NULL;
    
    	if ((ExpansionBase = (struct Library *)
    		OpenLibrary((unsigned char *)"expansion.library", 0L)) == NULL) {
    

    Moving the assignment out of the if-block's condition should be easier to read:

     struct Library *ExpansionBase = (struct Library *)
     	OpenLibrary((unsigned char *)"expansion.library", 0L);
     
     if ((ExpansionBase == NULL) {
     	return 0;
     }

    Also, are the type casts really necessary? If not, please drop them.

  • Patch Set #1, Line 154: 0

    Why return 0?

  • Patch Set #1, Line 169: register_spi_master(&spi_master_tf53x, NULL);

    This function returns something. Why not propagate the return value?

     return register_spi_master(&spi_master_tf53x, NULL);

To view, visit change 54944. To unsubscribe, or for help writing mail filters, visit settings.