| Message ID | b64802460ceadf02d96aef6b5d76173b0d98f8bb.1340360906.git.phrdina@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Fri, Jun 22, 2012 at 10:33 AM, Pavel Hrdina <phrdina@redhat.com> wrote: > After rewrite DSKCHG bit handling the test has to be updated. Now > is needed to seek to different track to clear DSKCHG bit. > > Signed-off-by: Pavel Hrdina <phrdina@redhat.com> > --- > tests/fdc-test.c | 29 +++++++++++++++++++++-------- > 1 files changed, 21 insertions(+), 8 deletions(-) > > diff --git a/tests/fdc-test.c b/tests/fdc-test.c > index 610e2f1..5280eff 100644 > --- a/tests/fdc-test.c > +++ b/tests/fdc-test.c > @@ -156,19 +156,20 @@ static uint8_t send_read_command(void) > return ret; > } > > -static void send_step_pulse(void) > +static void send_step_pulse(bool chg_cyl) > { > int drive = 0; > int head = 0; > - static int cyl = 0; > + int cyl = 0; > + > + if (chg_cyl) > + cyl = (cyl + 1) % 4; Missing braces, please use checkpatch.pl to avoid these issues. % 4 could be turned into & 3, maybe with a separate patch. > > floppy_send(CMD_SEEK); > floppy_send(head << 2 | drive); > g_assert(!get_irq(FLOPPY_IRQ)); > floppy_send(cyl); > ack_irq(); > - > - cyl = (cyl + 1) % 4; > } > > static uint8_t cmos_read(uint8_t reg) > @@ -195,8 +196,7 @@ static void test_no_media_on_start(void) > assert_bit_set(dir, DSKCHG); > dir = inb(FLOPPY_BASE + reg_dir); > assert_bit_set(dir, DSKCHG); > - send_step_pulse(); > - send_step_pulse(); > + send_step_pulse(1); > dir = inb(FLOPPY_BASE + reg_dir); > assert_bit_set(dir, DSKCHG); > dir = inb(FLOPPY_BASE + reg_dir); > @@ -227,7 +227,14 @@ static void test_media_change(void) > dir = inb(FLOPPY_BASE + reg_dir); > assert_bit_set(dir, DSKCHG); > > - send_step_pulse(); > + send_step_pulse(0); > + dir = inb(FLOPPY_BASE + reg_dir); > + assert_bit_set(dir, DSKCHG); > + dir = inb(FLOPPY_BASE + reg_dir); > + assert_bit_set(dir, DSKCHG); > + > + /* Step to next track should clear DSKCHG bit. */ > + send_step_pulse(1); > dir = inb(FLOPPY_BASE + reg_dir); > assert_bit_clear(dir, DSKCHG); > dir = inb(FLOPPY_BASE + reg_dir); > @@ -243,7 +250,13 @@ static void test_media_change(void) > dir = inb(FLOPPY_BASE + reg_dir); > assert_bit_set(dir, DSKCHG); > > - send_step_pulse(); > + send_step_pulse(0); > + dir = inb(FLOPPY_BASE + reg_dir); > + assert_bit_set(dir, DSKCHG); > + dir = inb(FLOPPY_BASE + reg_dir); > + assert_bit_set(dir, DSKCHG); > + > + send_step_pulse(1); > dir = inb(FLOPPY_BASE + reg_dir); > assert_bit_set(dir, DSKCHG); > dir = inb(FLOPPY_BASE + reg_dir); > -- > 1.7.7.6 > >
Am 24.06.2012 08:16, schrieb Blue Swirl: > On Fri, Jun 22, 2012 at 10:33 AM, Pavel Hrdina <phrdina@redhat.com> wrote: >> After rewrite DSKCHG bit handling the test has to be updated. Now >> is needed to seek to different track to clear DSKCHG bit. >> >> Signed-off-by: Pavel Hrdina <phrdina@redhat.com> >> --- >> tests/fdc-test.c | 29 +++++++++++++++++++++-------- >> 1 files changed, 21 insertions(+), 8 deletions(-) >> >> diff --git a/tests/fdc-test.c b/tests/fdc-test.c >> index 610e2f1..5280eff 100644 >> --- a/tests/fdc-test.c >> +++ b/tests/fdc-test.c >> @@ -156,19 +156,20 @@ static uint8_t send_read_command(void) >> return ret; >> } >> >> -static void send_step_pulse(void) >> +static void send_step_pulse(bool chg_cyl) >> { >> int drive = 0; >> int head = 0; >> - static int cyl = 0; >> + int cyl = 0; >> + >> + if (chg_cyl) >> + cyl = (cyl + 1) % 4; > > Missing braces, please use checkpatch.pl to avoid these issues. > > % 4 could be turned into & 3, maybe with a separate patch. I wouldn't do that. It's not something like a mask for a bitfield, but just a way to let counting start from the beginning after a while. If you have two way to express the same, choose the one that expresses the clearest what the real intention is. The compiler will optimise it (not that this optimisation in a test case like this really mattered...) Kevin
Am 22.06.2012 12:33, schrieb Pavel Hrdina: > After rewrite DSKCHG bit handling the test has to be updated. Now > is needed to seek to different track to clear DSKCHG bit. > > Signed-off-by: Pavel Hrdina <phrdina@redhat.com> > --- > tests/fdc-test.c | 29 +++++++++++++++++++++-------- > 1 files changed, 21 insertions(+), 8 deletions(-) > > diff --git a/tests/fdc-test.c b/tests/fdc-test.c > index 610e2f1..5280eff 100644 > --- a/tests/fdc-test.c > +++ b/tests/fdc-test.c > @@ -156,19 +156,20 @@ static uint8_t send_read_command(void) > return ret; > } > > -static void send_step_pulse(void) > +static void send_step_pulse(bool chg_cyl) > { > int drive = 0; > int head = 0; > - static int cyl = 0; > + int cyl = 0; > + > + if (chg_cyl) > + cyl = (cyl + 1) % 4; Why do you remove the static? Previously, the function would cycle through cylinders 1-4. Now it's always 0 if !chg_cyl and 1 if chg_cyl. I think you need to fix this. I also don't quite understand why you move the increment to here instead of leaving it as the bottom. It doesn't look wrong, but pointless. > > floppy_send(CMD_SEEK); > floppy_send(head << 2 | drive); > g_assert(!get_irq(FLOPPY_IRQ)); > floppy_send(cyl); > ack_irq(); > - > - cyl = (cyl + 1) % 4; > } > > static uint8_t cmos_read(uint8_t reg) > @@ -195,8 +196,7 @@ static void test_no_media_on_start(void) > assert_bit_set(dir, DSKCHG); > dir = inb(FLOPPY_BASE + reg_dir); > assert_bit_set(dir, DSKCHG); > - send_step_pulse(); > - send_step_pulse(); > + send_step_pulse(1); This is a bool parameter, so you should pass true/false instead of 0/1. Other than that, the test case additions look good and I'll accept it as sufficient for the DSKCHG handling change. It would be great, though, to add some more cases that test the implicit seeks during commands like READ (ideally one for each command that can seek). Kevin
On 06/25/2012 11:50 AM, Kevin Wolf wrote: > Am 22.06.2012 12:33, schrieb Pavel Hrdina: >> After rewrite DSKCHG bit handling the test has to be updated. Now >> is needed to seek to different track to clear DSKCHG bit. >> >> Signed-off-by: Pavel Hrdina<phrdina@redhat.com> >> --- >> tests/fdc-test.c | 29 +++++++++++++++++++++-------- >> 1 files changed, 21 insertions(+), 8 deletions(-) >> >> diff --git a/tests/fdc-test.c b/tests/fdc-test.c >> index 610e2f1..5280eff 100644 >> --- a/tests/fdc-test.c >> +++ b/tests/fdc-test.c >> @@ -156,19 +156,20 @@ static uint8_t send_read_command(void) >> return ret; >> } >> >> -static void send_step_pulse(void) >> +static void send_step_pulse(bool chg_cyl) >> { >> int drive = 0; >> int head = 0; >> - static int cyl = 0; >> + int cyl = 0; >> + >> + if (chg_cyl) >> + cyl = (cyl + 1) % 4; > Why do you remove the static? Previously, the function would cycle > through cylinders 1-4. Now it's always 0 if !chg_cyl and 1 if chg_cyl. I > think you need to fix this. > > I also don't quite understand why you move the increment to here instead > of leaving it as the bottom. It doesn't look wrong, but pointless. Now the functionality is different. I need seek to cylinder 0 or 1. After every change of media an actual cylinder is set to 0 and I need to try seek to cylinder 0 to test, that after this seek the DSKCHG bit is still set. Then I need to seek to different cylinder to reset the DSKCHG and for that seek to 1 is enough. I'll chagne the cyl = (cyl + 1) % 4; to cyl = 1; >> >> floppy_send(CMD_SEEK); >> floppy_send(head<< 2 | drive); >> g_assert(!get_irq(FLOPPY_IRQ)); >> floppy_send(cyl); >> ack_irq(); >> - >> - cyl = (cyl + 1) % 4; >> } >> >> static uint8_t cmos_read(uint8_t reg) >> @@ -195,8 +196,7 @@ static void test_no_media_on_start(void) >> assert_bit_set(dir, DSKCHG); >> dir = inb(FLOPPY_BASE + reg_dir); >> assert_bit_set(dir, DSKCHG); >> - send_step_pulse(); >> - send_step_pulse(); >> + send_step_pulse(1); > This is a bool parameter, so you should pass true/false instead of 0/1. > > Other than that, the test case additions look good and I'll accept it as > sufficient for the DSKCHG handling change. It would be great, though, to > add some more cases that test the implicit seeks during commands like > READ (ideally one for each command that can seek). > > Kevin I'll fix the bool parameter and then I'll create new patches for more test cases. Pavel
diff --git a/tests/fdc-test.c b/tests/fdc-test.c index 610e2f1..5280eff 100644 --- a/tests/fdc-test.c +++ b/tests/fdc-test.c @@ -156,19 +156,20 @@ static uint8_t send_read_command(void) return ret; } -static void send_step_pulse(void) +static void send_step_pulse(bool chg_cyl) { int drive = 0; int head = 0; - static int cyl = 0; + int cyl = 0; + + if (chg_cyl) + cyl = (cyl + 1) % 4; floppy_send(CMD_SEEK); floppy_send(head << 2 | drive); g_assert(!get_irq(FLOPPY_IRQ)); floppy_send(cyl); ack_irq(); - - cyl = (cyl + 1) % 4; } static uint8_t cmos_read(uint8_t reg) @@ -195,8 +196,7 @@ static void test_no_media_on_start(void) assert_bit_set(dir, DSKCHG); dir = inb(FLOPPY_BASE + reg_dir); assert_bit_set(dir, DSKCHG); - send_step_pulse(); - send_step_pulse(); + send_step_pulse(1); dir = inb(FLOPPY_BASE + reg_dir); assert_bit_set(dir, DSKCHG); dir = inb(FLOPPY_BASE + reg_dir); @@ -227,7 +227,14 @@ static void test_media_change(void) dir = inb(FLOPPY_BASE + reg_dir); assert_bit_set(dir, DSKCHG); - send_step_pulse(); + send_step_pulse(0); + dir = inb(FLOPPY_BASE + reg_dir); + assert_bit_set(dir, DSKCHG); + dir = inb(FLOPPY_BASE + reg_dir); + assert_bit_set(dir, DSKCHG); + + /* Step to next track should clear DSKCHG bit. */ + send_step_pulse(1); dir = inb(FLOPPY_BASE + reg_dir); assert_bit_clear(dir, DSKCHG); dir = inb(FLOPPY_BASE + reg_dir); @@ -243,7 +250,13 @@ static void test_media_change(void) dir = inb(FLOPPY_BASE + reg_dir); assert_bit_set(dir, DSKCHG); - send_step_pulse(); + send_step_pulse(0); + dir = inb(FLOPPY_BASE + reg_dir); + assert_bit_set(dir, DSKCHG); + dir = inb(FLOPPY_BASE + reg_dir); + assert_bit_set(dir, DSKCHG); + + send_step_pulse(1); dir = inb(FLOPPY_BASE + reg_dir); assert_bit_set(dir, DSKCHG); dir = inb(FLOPPY_BASE + reg_dir);
After rewrite DSKCHG bit handling the test has to be updated. Now is needed to seek to different track to clear DSKCHG bit. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- tests/fdc-test.c | 29 +++++++++++++++++++++-------- 1 files changed, 21 insertions(+), 8 deletions(-)