Re: [PATCH v6 3/7] ata: libata: Rename and cleanup ata_rwcmd_protocol()

Niklas Cassel
2022-12-30T07:44:06Z

On Tue, Nov 08, 2022 at 02:55:40PM +0900, Damien Le Moal wrote:
> Rename ata_rwcmd_protocol() to ata_set_rwcmd_protocol() to better
> reflect the fact that this function sets a task file command and
> protocol. The arguments order is also reversed and the function return
> type changed to a bool to indicate if the command and protocol were set
> corretly (instead of returning a completely arbitrary "-1" value.

correctly

>
> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> ---
> drivers/ata/libata-core.c | 28 +++++++++++++++-------------
> 1 file changed, 15 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 884ae73b11ea..6ee1cbac3ab0 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -574,17 +574,18 @@ static const u8 ata_rw_cmds[] = {
> };
>
> /**
> - * ata_rwcmd_protocol - set taskfile r/w commands and protocol
> - * @tf: command to examine and configure
> - * @dev: device tf belongs to
> + * ata_set_rwcmd_protocol - set taskfile r/w command and protocol
> + * @dev: target device for the taskfile
> + * @tf: taskfile to examine and configure
> *
> - * Examine the device configuration and tf->flags to calculate
> - * the proper read/write commands and protocol to use.
> + * Examine the device configuration and tf->flags to determine
> + * the proper read/write command and protocol to use for @tf.
> *
> * LOCKING:
> * caller.
> */
> -static int ata_rwcmd_protocol(struct ata_taskfile *tf, struct ata_device *dev)
> +static bool ata_set_rwcmd_protocol(struct ata_device *dev,
> + struct ata_taskfile *tf)
> {
> u8 cmd;
>
> @@ -607,11 +608,12 @@ static int ata_rwcmd_protocol(struct ata_taskfile *tf, struct ata_device *dev)
> }
>
> cmd = ata_rw_cmds[index + fua + lba48 + write];
> - if (cmd) {
> - tf->command = cmd;
> - return 0;
> - }
> - return -1;
> + if (!cmd)
> + return false;
> +
> + tf->command = cmd;
> +
> + return true;
> }
>
> /**
> @@ -744,7 +746,7 @@ int ata_build_rw_tf(struct ata_queued_cmd *qc, u64 block, u32 n_block,
> /* request too large even for LBA48 */
> return -ERANGE;
>
> - if (unlikely(ata_rwcmd_protocol(tf, dev) < 0))
> + if (unlikely(!ata_set_rwcmd_protocol(dev, tf)))
> return -EINVAL;
>
> tf->nsect = n_block & 0xff;
> @@ -762,7 +764,7 @@ int ata_build_rw_tf(struct ata_queued_cmd *qc, u64 block, u32 n_block,
> if (!lba_28_ok(block, n_block))
> return -ERANGE;
>
> - if (unlikely(ata_rwcmd_protocol(tf, dev) < 0))
> + if (unlikely(!ata_set_rwcmd_protocol(dev, tf)))
> return -EINVAL;
>
> /* Convert LBA to CHS */
> --
> 2.38.1
>

With or without small typo fixed:
Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>