On Sat, 2017-06-24 at 01:50 +0900, Minwoo Im wrote:
> - * Handles either 12 or 16-byte versions of the CDB.
> + * Handles either 12 16, or 32-byte versions of the CDB.
Please insert a comma between "12" and "16" to avoid that this comment
gets confusing.
> + if ((tf->protocol = ata_scsi_map_proto(cdb[1 + cdb_offset]))
> + == ATA_PROT_UNKNOWN) {
Have you verified this patch with checkpatch? The recommended style is
*not* to use assignments in the expression that controls an if-statement
and also if a comparison expression has to be split to keep the comparison
operator at the end of the first line.
> - shost->max_cmd_len = 16;
> + /*
> + * SPC says that CDB may have a fixed length of up to 16 bytes
> + * or variable length of between 12 and 260 bytes.
> + */
> + shost->max_cmd_len = 260;
As mentioned before, I think a maximum CDB length of 260 is overkill.
> /* Values for T10/04-262r7 */
> +#define ATA_32 0x1ff0 /* 32-byte pass-thru,
> service action */
> #define ATA_16 0x85 /* 16-byte pass-thru */
> #define ATA_12 0xa1 /* 12-byte pass-thru */
Defining ATA_32 just above ATA_12 and ATA_16 is misleading because
the latter two are CDB opcodes while the former is a service action
code. Please move the definition of the ATA_32 service action code
one line up such that it appears at the end of the list of already
defined service codes, namely this list:
/* values for variable length command */
#define XDREAD_32 0x03
#define XDWRITE_32 0x04
#define XPWRITE_32 0x06
#define XDWRITEREAD_32 0x07
#define READ_32 0x09
#define VERIFY_32 0x0a
#define WRITE_32 0x0b
#define WRITE_SAME_32 0x0d
Thanks,
Bart.
