I'm trying to describe the following behaviour of MCP4921 DAC.

SPI timing diagram

I'm using Basys 2 FPGA board as the master.

I've written the following code. Which works somehow.

module SPI_DAC_MCP4921(
input [11:0] Data ,
input Clk,
output SCK,     
output reg CS_L, //chip select low signal
output reg SPI   // serial data

assign SCK = Clk;
reg [4:0] counter=0;    //20 tick counter
parameter [3:0] write_command = 4'b0111; // Anot=0 BUF=1? GAnot=1 SHDNnot=1? 
//reg [15:0] write_command_register = {write_command, Data}; // 16 bit write register
always @ (posedge Clk) begin    
        if (counter==0 | counter < 4) begin   // first 4 bits
         CS_L <= 1'b0;                        // set CS_L active
         SPI <= write_command[3-counter];     // send 4 bit write command
         counter <= counter+1;            
        end else if (counter >=4 & counter <=15) begin  //12 bits
         SPI <= Data[15-counter];       //send data bits, starting with MSB
         counter <= counter+1;
        end else if (counter > 15 & counter <=18) begin    //deactivate CS_L
         CS_L <= 1'b1;
         counter <= counter+1; 
        end else if (counter ==19) counter <= 1'b0; // 20th tick reset counter
        //counter <= counter+1;

The problem is, while writing in HDL, I don't think about the actual hardware. I don't have a methodology. Should I sketch a state diagram? How should I sketch the flow? I mean I understand the combinational blocks; muxes, adders, comparators etc. and memory elements ff.s , registers. Combining these blocks for a more complex design proves to be difficult for me. I'm looking for improvements to my code, general style tips. Any help is appreciated.

  • 1
    \$\begingroup\$ Especially with complex logic, a picture is worth a thousand words. \$\endgroup\$ Jul 13, 2017 at 12:27
  • \$\begingroup\$ As a personal style, I would build the entire data stream of 16 bits prior to sending it; that decouples the command / data from the actual transmission and makes life a bit easier if I need to reuse the SPI driver when there is a somewhat different command / data format. \$\endgroup\$ Jul 13, 2017 at 14:13
  • \$\begingroup\$ //reg [15:0] write_command_register = {write_command, Data}; // 16 bit write register In this line of code I tried to do the thing you described, I think. By parametrizing the command and concatenating Data to it. It gives an error that Data is not constant. Should I create a data_ready signal and wire the Data to a register and set data_ready as the enable? \$\endgroup\$
    – zeb
    Jul 13, 2017 at 14:57

1 Answer 1


What frightens me most is not the code but the remake "Which works somehow."
What you have is a serial data protocol. That just screams for a shift register. Instead you are instancing multiplexers all over the place using an index with an mathematical operator like:

SPI <= write_command[3-counter]; 

I have built several SPI interfaces both in FPGA and ASIC. I always start with a statemachine. Obvious states are: IDLE, SHIFT and maybe a STOP. From IDLE to SHIFT you load the shift register and set the CS low. in SHIFT you shift that data out. I would normally go back to idle but you can add a STOP state to set the CS high again.
What you also might need is a clock divider. An SPI works at e.g. 1MHz but the code often runs at 10-300MHz.
I could find code I have in my archive, but I refrain from that because the best way to learn is to write it your self. Here are some code snippets (all written as I go thus, not checked)

localparam STATE_IDLE  = 2'b00,               
           STATE_SHIFT = 2'b01,
           STATE_STOP  = 2'b10;

case (state)
STATE_IDLE : // Here you need a signal that new data has arrived
    if (new_data)
       shift_out <= {write_command, Data}; 
       CS_L      <= 1'b0;
       count     <= 5'd0;  
       state     <= STATE_SHIFT;
    end // case IDLE

STATE_SHIFT: // Shift a bit out every second count
        count <= count + 5'd1;
        SCK   <= ~count[0]; // High after first count
        if (count==5'd31) // or 30? 32? Check in simulation! 
            state <= STATE_STOP;
            SCK   <= 1'b0;
        if (count[0]) // Second tick: output another bit
           shift_out <= {shift_out[14:0],1'b0};
    end // case SHIFT
        state <= STATE_IDLE;
        CS_L <= 1'b1


Note that at the count I am not sure which value I need to stop. I can think about it very long and hard and still come up with the wrong answer so I prefer to do what is shown here: put a comment in the code to myself that I need to check that. If it is wrong it is often wrong by one. This sounds strange but putting in the right amount from the start is not important. To me it is distracting from the main work which is to build the right structure.


Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.