Hi Finn,
Thanks again for the review!
Op vr 15 nov. 2019 om 03:09 schreef Finn Thain <fthain@xxxxxxxxxxxxxxxxxxx>:
On Thu, 14 Nov 2019, Kars de Jong wrote:
The FSC (NCR53CF9x-2 / SYM53CF9x-2) has a different family code than QLogic
or Emulex parts. This caused it to be detected as a FAS100A.
Unforunately, this meant the configuration of the CONFIG3 register was
incorrect. This causes data transfer issues with FAST-SCSI targets.
The FSC also has the CONFIG4 register. It can be used to enable a feature
called Active Negation which should always be enabled according to the data
manual.
Signed-off-by: Kars de Jong <jongk@xxxxxxxxxxxxxx>
Reviewed-by: Finn Thain <fthain@xxxxxxxxxxxxxxxxxxx>
This is not the best scope for this variable. You have to touch both lines
(declaration and initialization) anyway, so you can easily improve this.
Good point, I will move it to inside the "if (esp->rev) == FAST) {" block.
- /* Fast 236, AM53c974 or HME */
+ case FSC:
+ /* Fast 236, AM53c974, FSC or HME */
This comment merely re-states the logic in the case labels. If you don't
delete the comment, it has to be maintained along with the case labels.
Consequently this patch is longer than it needs to be.
Yes, I agree. I'll remove the comment.
You've added "(FSC)" and "(am53c974)" here, which is fine but you've
repeated yourself in the comment, "found on am53c974 and FSC chips". The
DRY principle applies here too (Don't Repeat Yourself).
I'll also remove that comment.
@@ -264,6 +271,7 @@ enum esp_rev {
ESP236,
FAS236,
PCSCSI, /* AM53c974 */
+ FSC, /* NCR/Symbios Logic FSC */
Is there a difference between "FSC" and "NCR/Symbios Logic FSC"?
No, the respective data manuals all name these variants as "FSC".
The reader has to infer that not all "FSC" chips are equivalent and that the
brand name is significant. Whereas, there is real value in putting a part
code here (as in the line above).
Agreed, I'll add the part code instead.
Kind regards,
Kars.
