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.
