On Tue, 12 Nov 2019, Kars de Jong wrote:
> The order of the definitions in the esp_rev enum is important. The values
> are used in comparisons for chip features.
>
> Add a comment to the enum explaining this.
>
> Also, the actual values for the enum fields are irrelevant, so remove the
> explicit values (suggested by Geert Uytterhoeven). This makes adding a new
> field in the middle of the enum easier.
>
> 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.
>
> Signed-off-by: Kars de Jong <jongk@xxxxxxxxxxxxxx>
> ---
> drivers/scsi/esp_scsi.c | 2 +-
> drivers/scsi/esp_scsi.h | 19 +++++++++++--------
> 2 files changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/scsi/esp_scsi.c b/drivers/scsi/esp_scsi.c
> index bb88995a12c7..4fc3eee3138b 100644
> --- a/drivers/scsi/esp_scsi.c
> +++ b/drivers/scsi/esp_scsi.c
> @@ -2373,10 +2373,10 @@ static const char *esp_chip_names[] = {
> "ESP100A",
> "ESP236",
> "FAS236",
> + "AM53C974",
> "FAS100A",
> "FAST",
> "FASHME",
> - "AM53C974",
> };
>
> static struct scsi_transport_template *esp_transport_template;
> diff --git a/drivers/scsi/esp_scsi.h b/drivers/scsi/esp_scsi.h
> index 91b32f2a1a1b..b96cbda03d2d 100644
> --- a/drivers/scsi/esp_scsi.h
> +++ b/drivers/scsi/esp_scsi.h
> @@ -257,15 +257,18 @@ struct esp_cmd_priv {
> };
> #define ESP_CMD_PRIV(CMD) ((struct esp_cmd_priv *)(&(CMD)->SCp))
>
> +/* NOTE: this enum is ordered based on chip features! */
Fair enough, that has been overlooked before.
> 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.
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.)
Aside from that:
Reviewed-by: Finn Thain <fthain@xxxxxxxxxxxxxxxxxxx>
--
