I don't see any problems with the code functionally, and the layout follows good practices for the most part. You chose meaningful names for the signals and parameters, and the parameters make your design scalable to different data widths. It also follows good practices using nonblocking assignments for the sequential logic. This is excellent for a beginner.

You should use a sized constant for the data reset value ('h0) in order to avoid synthesis lint warnings from some tools. I prefer the replicated concatenation style for this:{DATA_WIDTH{1'h0}}.

I think it is more conventional to omit the1'h1 == in comparisons to 1 for 1-bit signals. It makes the code a little cleaner, and all the tools do the right thing. For example:(wr_enable).

Similarly for comparisons to 0, omit the1'h0 ==, and use either! or~ (they are equivalent for 1-bit signals):(!s_rst_n_i)

You did a fantastic job of using a consistent layout style. The following comments are my preferences, but you might consider using them as well.

I use 4-space indentation because I think it is easier to see the different levels of your code's logic.

I prefer to place thebegin andend keywords on the same line as theif/else keywords in order to reduce vertical space and get more code on one screen. It also makes the branches of your code more evident.

Here is the code with the above suggestions:

`timescale 1ns / 1psmodule shift_reg # (    parameter integer DATA_WIDTH   = 16,    parameter integer DO_MSB_FIRST = "TRUE")(    input  wire                      clk_i,    input  wire                      s_rst_n_i,    input  wire                      enable,    input  wire                      wr_enable,    input  wire [DATA_WIDTH - 1 : 0] wr_data_i,    output wire                      serial_data_o);    localparam integer MSB = DATA_WIDTH - 1;    localparam integer LSB = 0;    localparam integer XSB = ("TRUE" == DO_MSB_FIRST) ? MSB : LSB;    reg [DATA_WIDTH - 1 : 0] parallel_data;    assign serial_data_o = (enable) ? parallel_data[XSB] : 1'hz;      always @ (posedge clk_i) begin        if (!s_rst_n_i) begin            parallel_data <= {DATA_WIDTH{1'h0}};        end else if (wr_enable) begin            parallel_data <= wr_data_i;        end else if (enable) begin            if ("TRUE" == DO_MSB_FIRST) begin                parallel_data <= {parallel_data[MSB - 1 : 0], 1'h0};            end else begin                parallel_data <= {1'h0, parallel_data[MSB : 1]};            end          end    endendmodule

• beginner
• verilog
• hdl