Re: [PATCH v3 2/2] esp_scsi: Add support for FSC chip

[Kars de Jong <jongk@linux-m68k.org>]

Hi Finn,

Thanks again for the review!

Op vr 15 nov. 2019 om 03:09 schreef Finn Thain <fthain@telegraphics.com.au>:
> 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@linux-m68k.org>
>
> Reviewed-by: Finn Thain <fthain@telegraphics.com.au>
>
> 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.