[6684] in linux-scsi channel archive
Re: patch-2.2.10.gz: review
daemon@ATHENA.MIT.EDU (dledford@redhat.com)
Wed Jun 16 04:57:19 1999
From: dledford@redhat.com
Date: Tue, 15 Jun 1999 14:58:10 -0400
To: Ulrich Windl <ulrich.windl@rz.uni-regensburg.de>
CC: linux-kernel@vger.rutgers.edu, linux-scsi@vger.rutgers.edu
Ulrich Windl wrote:
>
> Hello,
>
> browsing patch-2.2.10.gz some things fell in my eye that I have
> described in the attached text. I'm not sure that all are bugs, but
> maybe someone with better knowledge take a look (aic7xxx stuff).
> In linux/drivers/pci/oldproc.c the part of the patch seems strange:
>
> DEVICE( ADAPTEC, ADAPTEC_5800, "AIC-5800"),
> + DEVICE( ADAPTEC, ADAPTEC_3860, "AIC-7860"),
> DEVICE( ADAPTEC, ADAPTEC_7860, "AIC-7860"),
>
> Shouldn't it be something like
>
> + DEVICE( ADAPTEC, ADAPTEC_3860, "AIC-3860"),
>
> ?
No, it's right just the way it is. The internal Adaptec aic7xxx part name
does not always correspond directly to the announced/released product name.
> Another bit definition that looks strange is in
> linux/drivers/scsi/aic7xxx/aic7xxx.reg:
>
> +register CRCCONTROL1 {
> + address 0x09d
> + access_mode RW
> + bit CRCONSEEN 0x80 /* CRC ON Single Edge ENable */
> + bit CRCVALCHKEN 0x40 /* CRC Value Check Enable */
> + bit CRCENDCHKEN 0x20 /* CRC End Check Enable */
> + bit CRCREQCHKEN 0x10
> + bit TARGCRCENDEN 0x08 /* Enable End CRC transfer when target */
> + bit TARGCRCCNTEN 0x40 /* Enable CRC transfer when target */
> +}
>
> Shouldn´t it possibly be
>
> + bit TARGCRCCNTEN 0x04 /* Enable CRC transfer when target */
Yes, that one's a typo.
> Finally some cosmetics: In linux/drivers/scsi/aic7xxx_proc.c some lines read:
>
> +#ifdef CONFIG_AIC7XXX_TCQ_ON_BY_DEFAULT
> + size += sprintf(BLS, " TCQ Enabled By Default : Enabled\n");
> +#else
> + size += sprintf(BLS, " TCQ Enabled By Default : Disabled\n");
>
> Shouldn't these be simplified to
>
> +#ifdef CONFIG_AIC7XXX_TCQ_ON_BY_DEFAULT
> + size += sprintf(BLS, " TCQ Enabled By Default : Yes\n");
> +#else
> + size += sprintf(BLS, " TCQ Enabled By Default : No\n");
>
> ?
>
> Later the line
>
> + size += sprintf(BLS, " AIC7XXX_RESET_DELAY : %d\n", AIC7XXX_RESET_DELAY);
>
> could read
>
> + size += sprintf(BLS, " Delay after bus reset : %d\n", AIC7XXX_RESET_DELAY);
>
> as well. I mean: Either use the symbols from the source or use words
> that mean something even for dummies. "Tag Queue Depth" could also be
> "TCQ Depth" with the latest wording. The "Transinfo settings:" are
> really hard for dummies.
I'm not very concerned with these items. Most of the info in that file is for
my use (the transinfo is not intended to be normal person interpretable at
all, so I couldn't care less about it and readability, I'm concerned about
getting the info I need about SEEPROM configs out of it). Some of the
rewordings could be applied, and maybe I will put them in my next release, but
it certainly isn't a high priority for me.
--
Doug Ledford <dledford@redhat.com>
Opinions expressed are my own, but
they should be everybody's.
-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.rutgers.edu
