Re: [PATCH v6 2/7] ata: libata: Introduce ata_ncq_supported()

[Niklas Cassel <Niklas.Cassel@wdc.com>]

On Tue, Nov 08, 2022 at 02:55:39PM +0900, Damien Le Moal wrote:
> Introduce the inline helper function ata_ncq_supported() to test if a
> device supports NCQ commands. The function ata_ncq_enabled() is also
> rewritten using this new helper function.
> 
> 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>
> ---
>  include/linux/libata.h | 26 ++++++++++++++++++++------
>  1 file changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index af4953b95f76..58651f565b36 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -1690,21 +1690,35 @@ extern struct ata_device *ata_dev_next(struct ata_device *dev,
>  	     (dev) = ata_dev_next((dev), (link), ATA_DITER_##mode))
>  
>  /**
> - *	ata_ncq_enabled - Test whether NCQ is enabled
> - *	@dev: ATA device to test for
> + *	ata_ncq_supported - Test whether NCQ is supported
> + *	@dev: ATA device to test
>   *
>   *	LOCKING:
>   *	spin_lock_irqsave(host lock)
>   *
>   *	RETURNS:
> - *	1 if NCQ is enabled for @dev, 0 otherwise.
> + *	true if @dev supports NCQ, false otherwise.
>   */
> -static inline int ata_ncq_enabled(struct ata_device *dev)
> +static inline bool ata_ncq_supported(struct ata_device *dev)
>  {
>  	if (!IS_ENABLED(CONFIG_SATA_HOST))
>  		return 0;

Since you changed the return type to bool, and the function comment says
that you should return "false otherwise", perhaps change "return 0" to
"return false".

(Yes, they are technically the same, but it still makes me double check the
function's return type every time I see a "return 0" where I expected a bool,
since perhaps I was reading too quickly and overlooked something.)

> -	return (dev->flags & (ATA_DFLAG_PIO | ATA_DFLAG_NCQ_OFF |
> -			      ATA_DFLAG_NCQ)) == ATA_DFLAG_NCQ;
> +	return (dev->flags & (ATA_DFLAG_PIO | ATA_DFLAG_NCQ)) == ATA_DFLAG_NCQ;
> +}
> +
> +/**
> + *	ata_ncq_enabled - Test whether NCQ is enabled
> + *	@dev: ATA device to test
> + *
> + *	LOCKING:
> + *	spin_lock_irqsave(host lock)
> + *
> + *	RETURNS:
> + *	true if NCQ is enabled for @dev, false otherwise.
> + */
> +static inline bool ata_ncq_enabled(struct ata_device *dev)
> +{
> +	return ata_ncq_supported(dev) && !(dev->flags & ATA_DFLAG_NCQ_OFF);
>  }
>  
>  static inline bool ata_fpdma_dsm_supported(struct ata_device *dev)
> -- 
> 2.38.1
> 

With the small nit:
Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>