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 <kw...@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
