Re: [Qemu-block] [Qemu-devel] [PATCH 5/8] fdc: Code cleanup in fdctrl_write_data()

[John Snow]

On 05/19/2015 11:35 AM, Kevin Wolf wrote:
> Factor out a few common lines of code, reformat, improve comments.
> 
> Signed-off-by: Kevin Wolf <address@hidden>
> ---
>  hw/block/fdc.c | 62 
> +++++++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 38 insertions(+), 24 deletions(-)
> 
> diff --git a/hw/block/fdc.c b/hw/block/fdc.c
> index a13e0ce..cbf7abf 100644
> --- a/hw/block/fdc.c
> +++ b/hw/block/fdc.c
> @@ -1942,14 +1942,16 @@ static void fdctrl_handle_relative_seek_out(FDCtrl 
> *fdctrl, int direction)
>  /*
>   * Handlers for the execution phase of each command
>   */
> -static const struct {
> +typedef struct FDCtrlCommand {
>      uint8_t value;
>      uint8_t mask;
>      const char* name;
>      int parameters;
>      void (*handler)(FDCtrl *fdctrl, int direction);
>      int direction;
> -} handlers[] = {
> +} FDCtrlCommand;
> +
> +static const FDCtrlCommand handlers[] = {
>      { FD_CMD_READ, 0x1f, "READ", 8, fdctrl_start_transfer, FD_DIR_READ },
>      { FD_CMD_WRITE, 0x3f, "WRITE", 8, fdctrl_start_transfer, FD_DIR_WRITE },
>      { FD_CMD_SEEK, 0xff, "SEEK", 2, fdctrl_handle_seek },
> @@ -1986,9 +1988,19 @@ static const struct {
>  /* Associate command to an index in the 'handlers' array */
>  static uint8_t command_to_handler[256];
>  
> +static const FDCtrlCommand *get_command(uint8_t cmd)
> +{
> +    int idx;
> +
> +    idx = command_to_handler[cmd];
> +    FLOPPY_DPRINTF("%s command\n", handlers[idx].name);
> +    return &handlers[idx];
> +}
> +
>  static void fdctrl_write_data(FDCtrl *fdctrl, uint32_t value)
>  {
>      FDrive *cur_drv;
> +    const FDCtrlCommand *cmd;
>      uint32_t pos;
>  
>      /* Reset mode */
> @@ -2002,13 +2014,20 @@ static void fdctrl_write_data(FDCtrl *fdctrl, 
> uint32_t value)
>      }
>      fdctrl->dsr &= ~FD_DSR_PWRDOWN;
>  
> +    FLOPPY_DPRINTF("%s: %02x\n", __func__, value);
> +
> +    /* If data_len spans multiple sectors, the current position in the FIFO
> +     * wraps around while fdctrl->data_pos is the real position in the whole
> +     * request. */
> +    pos = fdctrl->data_pos++;
> +    pos %= FD_SECTOR_LEN;
> +    fdctrl->fifo[pos] = value;
> +
>      switch (fdctrl->phase) {
>      case FD_PHASE_EXECUTION:
>          assert(fdctrl->msr & FD_MSR_NONDMA);
> +
>          /* FIFO data write */
> -        pos = fdctrl->data_pos++;
> -        pos %= FD_SECTOR_LEN;
> -        fdctrl->fifo[pos] = value;
>          if (pos == FD_SECTOR_LEN - 1 ||
>              fdctrl->data_pos == fdctrl->data_len) {
>              cur_drv = get_cur_drv(fdctrl);
> @@ -2024,41 +2043,36 @@ static void fdctrl_write_data(FDCtrl *fdctrl, 
> uint32_t value)
>                  break;
>              }
>          }
> -        /* Switch from transfer mode to status mode
> -         * then from status mode to command mode
> -         */
> -        if (fdctrl->data_pos == fdctrl->data_len)
> +
> +        /* Switch to result phase when done with the transfer */
> +        if (fdctrl->data_pos == fdctrl->data_len) {
>              fdctrl_stop_transfer(fdctrl, 0x00, 0x00, 0x00);
> +        }
>          break;
>  
>      case FD_PHASE_COMMAND:
>          assert(!(fdctrl->msr & FD_MSR_NONDMA));
>  
> -        if (fdctrl->data_pos == 0) {
> -            /* Command */
> -            pos = command_to_handler[value & 0xff];
> -            FLOPPY_DPRINTF("%s command\n", handlers[pos].name);
> -            fdctrl->data_len = handlers[pos].parameters + 1;
> +        if (fdctrl->data_pos == 1) {

This reads more awkwardly than the previous ifz, but it's not
like I have a better idea. (I just had a momentary pause of "Why 1?")

> +            /* The first byte specifies the command. Now we start reading
> +             * as many parameters as this command requires. */
> +            cmd = get_command(value);
> +            fdctrl->data_len = cmd->parameters + 1;
>              fdctrl->msr |= FD_MSR_CMDBUSY;
>          }
>  
> -        FLOPPY_DPRINTF("%s: %02x\n", __func__, value);
> -        pos = fdctrl->data_pos++;
> -        pos %= FD_SECTOR_LEN;
> -        fdctrl->fifo[pos] = value;
>          if (fdctrl->data_pos == fdctrl->data_len) {
> -            /* We now have all parameters
> -             * and will be able to treat the command
> -             */
> +            /* We have all parameters now, execute the command */
>              fdctrl->phase = FD_PHASE_EXECUTION;
> +
>              if (fdctrl->data_state & FD_STATE_FORMAT) {
>                  fdctrl_format_sector(fdctrl);
>                  break;
>              }
>  
> -            pos = command_to_handler[fdctrl->fifo[0] & 0xff];
> -            FLOPPY_DPRINTF("treat %s command\n", handlers[pos].name);
> -            (*handlers[pos].handler)(fdctrl, handlers[pos].direction);
> +            cmd = get_command(fdctrl->fifo[0]);
> +            FLOPPY_DPRINTF("Calling handler for '%s'\n", cmd->name);
> +            cmd->handler(fdctrl, cmd->direction);
>          }
>          break;
>  
> 

Reviewed-by: John Snow <address@hidden>