| Message ID | 20191112185710.23988-2-jongk@linux-m68k.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Some esp_scsi updates | expand |
On Tue, 12 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. Add comments to the > enum explaining this. > > Signed-off-by: Kars de Jong <jongk@linux-m68k.org> > --- > drivers/scsi/esp_scsi.c | 2 +- > drivers/scsi/esp_scsi.h | 19 +++++++++++-------- > 2 files changed, 12 insertions(+), 9 deletions(-) > > 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..b96cbda03d2d 100644 > --- a/drivers/scsi/esp_scsi.h > +++ b/drivers/scsi/esp_scsi.h > @@ -257,15 +257,18 @@ struct esp_cmd_priv { > }; > #define ESP_CMD_PRIV(CMD) ((struct esp_cmd_priv *)(&(CMD)->SCp)) > > +/* NOTE: this enum is ordered based on chip features! */ Fair enough, that has been overlooked before. > 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, > + /* Chips below this line use ESP_CONFIG3_FSCSI to enable FAST SCSI */ > + FAS236, > + PCSCSI, /* AM53c974 */ > + /* Chips below this line use ESP_CONFIG3_FAST to enable FAST SCSI */ > + FAS100A, > + FAST, > + FASHME, > }; > > struct esp_cmd_entry { > FAS100A, FAST and FASHME are below both lines, which is a bit confusing. In general, I don't like to see comments that merely re-state the explicit logic in the algorithm. Such comments add no value. (At best this redundancy creates a maintenance burden and at worst the commentary becomes neglected until it creates contradictions.) Aside from that: Reviewed-by: Finn Thain <fthain@telegraphics.com.au> --
Hi Finn, Thanks for your review! Op wo 13 nov. 2019 om 00:07 schreef Finn Thain <fthain@telegraphics.com.au>: > On Tue, 12 Nov 2019, Kars de Jong wrote: > > 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. Add comments to the > > enum explaining this. > > > > 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, > > + /* Chips below this line use ESP_CONFIG3_FSCSI to enable FAST SCSI */ > > + FAS236, > > + PCSCSI, /* AM53c974 */ > > + /* Chips below this line use ESP_CONFIG3_FAST to enable FAST SCSI */ > > + FAS100A, > > + FAST, > > + FASHME, > > }; > > > > struct esp_cmd_entry { > > > > FAS100A, FAST and FASHME are below both lines, which is a bit confusing. Hmm, you're right. But I don't really know how to solve that. But if you think the initial comment is enough to trigger people to investigate the algorithm, I can remove them. > In general, I don't like to see comments that merely re-state the explicit > logic in the algorithm. Such comments add no value. > > (At best this redundancy creates a maintenance burden and at worst the > commentary becomes neglected until it creates contradictions.) Unfortunately the algorithm isn't very obvious here (well, not to me at least). Kind regards, Kars.
On Tue, Nov 12, 2019 at 07:57:09PM +0100, 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. Yikes. Wouldn't it make much more sense to have a feature bitmap?
Hi Christoph! Op wo 13 nov. 2019 om 15:22 schreef Christoph Hellwig <hch@infradead.org>: > > On Tue, Nov 12, 2019 at 07:57:09PM +0100, 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. > > Yikes. Wouldn't it make much more sense to have a feature bitmap? Yes, I mentioned that in my cover letter already, and it was discussed a bit on the Linux/m68k mailing list. It may be hard to get that tested though, most of the users are legacy architectures with a probably very limited number of users. But if a mostly review-based process is good enough, I am certainly willing to change it. Kind regards, Kars.
On Wed, 13 Nov 2019, Kars de Jong wrote: > > > > FAS100A, FAST and FASHME are below both lines, which is a bit > > confusing. > > Hmm, you're right. But I don't really know how to solve that. But if you > think the initial comment is enough to trigger people to investigate the > algorithm, I can remove them. > Yes, please. The initial "NOTE!" that you added is sufficient, IMHO. --
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..b96cbda03d2d 100644 --- a/drivers/scsi/esp_scsi.h +++ b/drivers/scsi/esp_scsi.h @@ -257,15 +257,18 @@ 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, + /* Chips below this line use ESP_CONFIG3_FSCSI to enable FAST SCSI */ + FAS236, + PCSCSI, /* AM53c974 */ + /* Chips below this line use ESP_CONFIG3_FAST to enable FAST SCSI */ + 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. Add comments to the enum explaining this. Signed-off-by: Kars de Jong <jongk@linux-m68k.org> --- drivers/scsi/esp_scsi.c | 2 +- drivers/scsi/esp_scsi.h | 19 +++++++++++-------- 2 files changed, 12 insertions(+), 9 deletions(-)