Hi Finn,
Thanks for your review!
Op wo 13 nov. 2019 om 00:07 schreef Finn Thain <fthain@xxxxxxxxxxxxxxxxxxx>:
> On Tue, 12 Nov 2019, Kars de Jong wrote:
> > Finally, move the PCSCSI definition to the right place in the enum. In its
> > previous location, at the end of the enum, the wrong values are written to
> > the CONFIG3 register when used with FAST-SCSI targets. Add comments to the
> > enum explaining this.
> >
> > enum esp_rev {
> > - ESP100 = 0x00, /* NCR53C90 - very broken */
> > - ESP100A = 0x01, /* NCR53C90A */
> > - ESP236 = 0x02,
> > - FAS236 = 0x03,
> > - FAS100A = 0x04,
> > - FAST = 0x05,
> > - FASHME = 0x06,
> > - PCSCSI = 0x07, /* AM53c974 */
> > + ESP100, /* NCR53C90 - very broken */
> > + ESP100A, /* NCR53C90A */
> > + ESP236,
> > + /* Chips below this line use ESP_CONFIG3_FSCSI to enable FAST SCSI */
> > + FAS236,
> > + PCSCSI, /* AM53c974 */
> > + /* Chips below this line use ESP_CONFIG3_FAST to enable FAST SCSI */
> > + FAS100A,
> > + FAST,
> > + FASHME,
> > };
> >
> > struct esp_cmd_entry {
> >
>
> FAS100A, FAST and FASHME are below both lines, which is a bit confusing.
Hmm, you're right. But I don't really know how to solve that. But if
you think the initial comment is enough to trigger people to
investigate the algorithm, I can remove them.
> In general, I don't like to see comments that merely re-state the explicit
> logic in the algorithm. Such comments add no value.
>
> (At best this redundancy creates a maintenance burden and at worst the
> commentary becomes neglected until it creates contradictions.)
Unfortunately the algorithm isn't very obvious here (well, not to me at least).
Kind regards,
Kars.
