| Message ID | 20191114222518.2441-2-jongk@linux-m68k.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2,1/2] esp_scsi: Correct ordering of PCSCSI definition in esp_rev enum | expand |
For simplicity, the entire patch series would normally show the same version number (v3). Also, the series would normally be a thread unto itself, rather than a sub-thread. On Thu, 14 Nov 2019, Kars de Jong wrote: > The order of the definitions in the esp_rev enum is important. The values > are used in comparisons for chip features. > > Add a comment to the enum explaining this. > > Also, the actual values for the enum fields are irrelevant, so remove the > explicit values (suggested by Geert Uytterhoeven). This makes adding a new > field in the middle of the enum easier. > > Finally, move the PCSCSI definition to the right place in the enum. In its > previous location, at the end of the enum, the wrong values are written to > the CONFIG3 register when used with FAST-SCSI targets. > > Signed-off-by: Kars de Jong <jongk@linux-m68k.org> This is Hannes' code so I'll leave it up to him to review this change. I presume this is untested on PCSCSI hardware. ISTR that there's an emulator for that board somewhere...
Hi Finn! Op vr 15 nov. 2019 om 03:13 schreef Finn Thain <fthain@telegraphics.com.au>: > For simplicity, the entire patch series would normally show the same > version number (v3). OK, thanks. > Also, the series would normally be a thread unto itself, rather than a sub-thread. OK, I followed an example from the git-send-email manual ("which avoids breaking threads to provide a new patch series"). I found the relevant convention in "Submitting patches", I didn't notice that before. Thanks. > ... > > This is Hannes' code so I'll leave it up to him to review this change. > > I presume this is untested on PCSCSI hardware. ISTR that there's an > emulator for that board somewhere... Yes, I don't have any, so I could not test it. QEMU emulates it, but it doesn't care about CONFIG3 etc. at all. Thanks for your reviews! Kind regards, Kars.
diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c index bb88995a12c7..4fc3eee3138b 100644 --- a/drivers/scsi/esp_scsi.c +++ b/drivers/scsi/esp_scsi.c @@ -2373,10 +2373,10 @@ static const char *esp_chip_names[] = { "ESP100A", "ESP236", "FAS236", + "AM53C974", "FAS100A", "FAST", "FASHME", - "AM53C974", }; static struct scsi_transport_template *esp_transport_template; diff --git a/drivers/scsi/esp_scsi.h b/drivers/scsi/esp_scsi.h index 91b32f2a1a1b..f764d64e1f25 100644 --- a/drivers/scsi/esp_scsi.h +++ b/drivers/scsi/esp_scsi.h @@ -257,15 +257,16 @@ struct esp_cmd_priv { }; #define ESP_CMD_PRIV(CMD) ((struct esp_cmd_priv *)(&(CMD)->SCp)) +/* NOTE: this enum is ordered based on chip features! */ enum esp_rev { - ESP100 = 0x00, /* NCR53C90 - very broken */ - ESP100A = 0x01, /* NCR53C90A */ - ESP236 = 0x02, - FAS236 = 0x03, - FAS100A = 0x04, - FAST = 0x05, - FASHME = 0x06, - PCSCSI = 0x07, /* AM53c974 */ + ESP100, /* NCR53C90 - very broken */ + ESP100A, /* NCR53C90A */ + ESP236, + FAS236, + PCSCSI, /* AM53c974 */ + FAS100A, + FAST, + FASHME, }; struct esp_cmd_entry {
The order of the definitions in the esp_rev enum is important. The values are used in comparisons for chip features. Add a comment to the enum explaining this. Also, the actual values for the enum fields are irrelevant, so remove the explicit values (suggested by Geert Uytterhoeven). This makes adding a new field in the middle of the enum easier. Finally, move the PCSCSI definition to the right place in the enum. In its previous location, at the end of the enum, the wrong values are written to the CONFIG3 register when used with FAST-SCSI targets. Signed-off-by: Kars de Jong <jongk@linux-m68k.org> --- drivers/scsi/esp_scsi.c | 2 +- drivers/scsi/esp_scsi.h | 17 +++++++++-------- 2 files changed, 10 insertions(+), 9 deletions(-)