| Message ID | 20220815141704.1178489-1-niklas.cassel@wdc.com |
|---|---|
| State | New |
| Headers | show |
| Series | ata: libata-core: improve parameter names for ata_dev_set_feature() | expand |
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@wdc.com> > --- > 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);
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 * - * 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);
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@wdc.com> --- drivers/ata/libata-core.c | 19 +++++++++---------- drivers/ata/libata.h | 2 +- 2 files changed, 10 insertions(+), 11 deletions(-)