[5/8] fdc: Code cleanup in fdctrl_write_data()

Factor out a few common lines of code, reformat, improve comments.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 hw/block/fdc.c | 62 +++++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 38 insertions(+), 24 deletions(-)

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 <kwolf@redhat.com>
> ---
>  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 <jsnow@redhat.com>

Am 19.05.2015 um 22:40 hat John Snow geschrieben:
> 
> 
> 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 <kwolf@redhat.com>
> > ---
> >  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?")

Hm, I think we can assert fdctrl->data_pos < FD_SECTOR_LEN (this is the
command phase and commands only have a few bytes of parameters) and
therefore check for pos == 0 here, if you think that's better.

Kevin