Hi Finn, Thanks for your review! Op wo 13 nov. 2019 om 00:07 schreef Finn Thain <fthain@xxxxxxxxxxxxxxxxxxx>:
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.
