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

From: Geert Uytterhoeven <geert@linux-m68k.org>
To: Kars de Jong <jongk@linux-m68k.org>
Cc: Michael Schmitz <schmitzmic@gmail.com>,
	Finn Thain <fthain@telegraphics.com.au>,
	"Linux/m68k" <linux-m68k@vger.kernel.org>
Subject: Re: [PATCH] esp_scsi: Add support for FSC chip
Date: Wed, 30 Oct 2019 09:45:05 +0100	[thread overview]
Message-ID: <CAMuHMdWMzvrVbfYDOZQcUK14h2Nzd7EqUJkwe3C+N3YUHYvyrQ@mail.gmail.com> (raw)
In-Reply-To: <CACz-3rjP9ozpqb9ypGjJTr=c=ka252KF2xALWr8KHLCvGc1z-Q@mail.gmail.com>

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 ;-)

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.

> > > I'm guessing that you've placed it here because of the esp->rev >= FAS236
> > > tests that appear in a couple of places relating to sync transfer period.
> > > Might be worth mentioning that in the commit log.
> >
> > Good catch. I think these tests would need to be changed to esp->rev
> >  >=FSC to reflect the fact that the FSC has the same CONFIG3 settings as
> > the FAS236.
>
> 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.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds