On 2022/08/15 7:17, Niklas Cassel wrote:
> ata_dev_set_feature() is currently used for enabling/disabling any ATA
> feature, e.g. SETFEATURES_SPINUP and SETFEATURE_SENSE_DATA, i.e. it is
> not only used to enable/disable SATA specific features.
>
> For most features, the enable/disable bit is specified in the subcommand
> specific field "count".
> It is only for the specific subcommands "Enable SATA feature" (0x10) and
> "Disable SATA feature" (0x90) where the field "count" is used to specify
> a feature instead of enable/disable. The parameter names for this
> function are thus quite misleading.
>
> Rename the parameter names to be more generic and in line with ACS-5,
> and remove the references to "SATA FEATURES" in the kernel-doc.
>
> Signed-off-by: Niklas Cassel <niklas.cassel@xxxxxxx>
> ---
> drivers/ata/libata-core.c | 19 +++++++++----------
> drivers/ata/libata.h | 2 +-
> 2 files changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 826d41f341e4..f737d32ceb82 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -4324,13 +4324,12 @@ static unsigned int ata_dev_set_xfermode(struct ata_device *dev)
> }
>
> /**
> - * ata_dev_set_feature - Issue SET FEATURES - SATA FEATURES
> + * ata_dev_set_feature - Issue SET FEATURES
> * @dev: Device to which command will be sent
> - * @enable: Whether to enable or disable the feature
> - * @feature: The sector count represents the feature to set
> + * @subcmd: The SET FEATURES subcommand to be sent
> + * @count: The sector count represents a subcommand specific action
Why not call this one "action" ? Better have the API parameter name represent
the meaning of the value rather than the cdb field used to pack it.
> *
> - * Issue SET FEATURES - SATA FEATURES command to device @dev
> - * on port @ap with sector count
> + * Issue SET FEATURES command to device @dev on port @ap with sector count
> *
> * LOCKING:
> * PCI/etc. bus probe sem.
> @@ -4338,23 +4337,23 @@ static unsigned int ata_dev_set_xfermode(struct ata_device *dev)
> * RETURNS:
> * 0 on success, AC_ERR_* mask otherwise.
> */
> -unsigned int ata_dev_set_feature(struct ata_device *dev, u8 enable, u8 feature)
> +unsigned int ata_dev_set_feature(struct ata_device *dev, u8 subcmd, u8 count)
> {
> struct ata_taskfile tf;
> unsigned int err_mask;
> unsigned int timeout = 0;
>
> /* set up set-features taskfile */
> - ata_dev_dbg(dev, "set features - SATA features\n");
> + ata_dev_dbg(dev, "set features\n");
>
> ata_tf_init(dev, &tf);
> tf.command = ATA_CMD_SET_FEATURES;
> - tf.feature = enable;
> + tf.feature = subcmd;
> tf.flags |= ATA_TFLAG_ISADDR | ATA_TFLAG_DEVICE;
> tf.protocol = ATA_PROT_NODATA;
> - tf.nsect = feature;
> + tf.nsect = count;
>
> - if (enable == SETFEATURES_SPINUP)
> + if (subcmd == SETFEATURES_SPINUP)
> timeout = ata_probe_timeout ?
> ata_probe_timeout * 1000 : SETFEATURES_SPINUP_TIMEOUT;
> err_mask = ata_exec_internal(dev, &tf, NULL, DMA_NONE, NULL, 0, timeout);
> diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
> index 98bc8649c63f..ccc8ba037cb1 100644
> --- a/drivers/ata/libata.h
> +++ b/drivers/ata/libata.h
> @@ -64,7 +64,7 @@ extern int ata_dev_configure(struct ata_device *dev);
> extern int sata_down_spd_limit(struct ata_link *link, u32 spd_limit);
> extern int ata_down_xfermask_limit(struct ata_device *dev, unsigned int sel);
> extern unsigned int ata_dev_set_feature(struct ata_device *dev,
> - u8 enable, u8 feature);
> + u8 subcmd, u8 count);
> extern void ata_qc_free(struct ata_queued_cmd *qc);
> extern void ata_qc_issue(struct ata_queued_cmd *qc);
> extern void __ata_qc_complete(struct ata_queued_cmd *qc);
--
Damien Le Moal
Western Digital Research
