Re: [PATCH] esp_scsi: Add support for FSC chip

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

Hi Geert!

Op wo 30 okt. 2019 om 09:45 schreef Geert Uytterhoeven <geert@linux-m68k.org>:
>
> Hi Kars,
>
> Thanks for your patch!
>
> Initially I wondere how the Apple MESH SCSI (also 53cf94 based) on
> my CHRP Longtrail used to work with this, until I realized my Longtrail
> died a few years before the new ESP SCSI driver was written ;-)

Yes, it's been a while :-)

> On Wed, Oct 30, 2019 at 8:26 AM Kars de Jong <jongk@linux-m68k.org> wrote:
> > Op wo 30 okt. 2019 om 02:06 schreef Michael Schmitz <schmitzmic@gmail.com>:
> > > >> @@ -262,10 +263,11 @@ enum esp_rev {
> > > >>      ESP100A    = 0x01,  /* NCR53C90A */
> > > >>      ESP236     = 0x02,
> > > >>      FAS236     = 0x03,
> > > >> -    FAS100A    = 0x04,
> > > >> -    FAST       = 0x05,
> > > >> -    FASHME     = 0x06,
> > > >> -    PCSCSI     = 0x07,  /* AM53c974 */
> > > >> +    FSC        = 0x04,  /* NCR/Symbios Logic FSC */
> > > >> +    FAS100A    = 0x05,
> > > >> +    FAST       = 0x06,
> > > >> +    FASHME     = 0x07,
> > > >> +    PCSCSI     = 0x08,  /* AM53c974 */
>
> Is anything caring about the actual values?
> Doesn't look like that, so you can just drop the values, and let standard C
> enum handling assign the numbers. That way you don't have to renumber
> when adding a new part.

Nice one. That's indeed even better.

> > Um, no, that would break the FAS236. FSC is defined after FAS236. It's
> > import that its values is lower than FAS100A and greater or equal than
> > FAS236. And as I wrote in reply to Finn, I think PCSCSI falls in the
> > same category.
>
> You definitely want to add a comment like "all below use the same CONFIG3
> settings", to avoid nasty surprises for future additions (if any).
> Using feature bits might be even better, but may not be worthwhile, since
> there seems to be only a single "esp->rev > ..." check.

No, there are several more actually:
 * "esp->rev > ESP100A" which basically means "HAS_CONFIG3".
 * "esp->rev < ESP236" which basically means "!HAS_FAST_CLOCK".
 * "esp->rev >= FAS236" which basically means "HAS_FAST_CLOCK"
 * "esp->rev >= FAS100A" which basically means "HAS_OLD_CONFIG3"

So, perhaps having feature bits is not a bad idea at all...

Kind regards,

Kars.