Re: [PATCH v2 1/2] ata: libata-sff: refactor ata_sff_set_devctl()

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

On 2022/02/14 0:10, Sergey Shtylyov wrote:
> Commit 41dec29bcb05 ("libata: introduce sff_set_devctl() method") left some
> clumsy checks surrounding calls to ata_sff_set_devctl() which Jeff Garzik
> suggested to factor out...  and I never followed up. :-(
> 
> At last, refactor ata_sff_set_devctl() to include the repetitive checks and
> return a 'bool' result indicating if the device control register exists or
> not.
> 
> While at it, further update the 'kernel-doc' comment -- the device control
> register has never been a part of the taskfile, despite what Jeff and co.
> think! :-)
> 
> Signed-off-by: Sergey Shtylyov <s.shtylyov@omp.ru>

This patch has "v2"... I do not recall seeing a v1 and the cover letter has no
changelog (and no v2 tag)... Is this a new series ?

> ---
>  drivers/ata/libata-sff.c | 30 +++++++++++++++---------------
>  1 file changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
> index 546b1f73ede5..3fb5bd4de50c 100644
> --- a/drivers/ata/libata-sff.c
> +++ b/drivers/ata/libata-sff.c
> @@ -265,20 +265,26 @@ EXPORT_SYMBOL_GPL(ata_sff_wait_ready);
>   *	@ap: port where the device is
>   *	@ctl: value to write
>   *
> - *	Writes ATA taskfile device control register.
> + *	Writes ATA device control register.
>   *
> - *	Note: may NOT be used as the sff_set_devctl() entry in
> - *	ata_port_operations.
> + *	RETURN:
> + *	true if the register exists, false if not.
>   *
>   *	LOCKING:
>   *	Inherited from caller.
>   */
> -static void ata_sff_set_devctl(struct ata_port *ap, u8 ctl)
> +static bool ata_sff_set_devctl(struct ata_port *ap, u8 ctl)
>  {
> -	if (ap->ops->sff_set_devctl)
> +	if (ap->ops->sff_set_devctl) {
>  		ap->ops->sff_set_devctl(ap, ctl);
> -	else
> +		return true;
> +	}
> +	if (ap->ioaddr.ctl_addr) {
>  		iowrite8(ctl, ap->ioaddr.ctl_addr);
> +		return true;
> +	}
> +
> +	return false;
>  }
>  
>  /**
> @@ -357,8 +363,6 @@ static void ata_dev_select(struct ata_port *ap, unsigned int device,
>   */
>  void ata_sff_irq_on(struct ata_port *ap)
>  {
> -	struct ata_ioports *ioaddr = &ap->ioaddr;
> -
>  	if (ap->ops->sff_irq_on) {
>  		ap->ops->sff_irq_on(ap);
>  		return;
> @@ -367,8 +371,7 @@ void ata_sff_irq_on(struct ata_port *ap)
>  	ap->ctl &= ~ATA_NIEN;
>  	ap->last_ctl = ap->ctl;
>  
> -	if (ap->ops->sff_set_devctl || ioaddr->ctl_addr)
> -		ata_sff_set_devctl(ap, ap->ctl);
> +	ata_sff_set_devctl(ap, ap->ctl);
>  	ata_wait_idle(ap);
>  
>  	if (ap->ops->sff_irq_clear)
> @@ -1662,8 +1665,7 @@ void ata_sff_freeze(struct ata_port *ap)
>  	ap->ctl |= ATA_NIEN;
>  	ap->last_ctl = ap->ctl;
>  
> -	if (ap->ops->sff_set_devctl || ap->ioaddr.ctl_addr)
> -		ata_sff_set_devctl(ap, ap->ctl);
> +	ata_sff_set_devctl(ap, ap->ctl);
>  
>  	/* Under certain circumstances, some controllers raise IRQ on
>  	 * ATA_NIEN manipulation.  Also, many controllers fail to mask
> @@ -2061,10 +2063,8 @@ void ata_sff_postreset(struct ata_link *link, unsigned int *classes)
>  		return;
>  
>  	/* set up device control */
> -	if (ap->ops->sff_set_devctl || ap->ioaddr.ctl_addr) {
> -		ata_sff_set_devctl(ap, ap->ctl);
> +	if (ata_sff_set_devctl(ap, ap->ctl))
>  		ap->last_ctl = ap->ctl;
> -	}
>  }
>  EXPORT_SYMBOL_GPL(ata_sff_postreset);
>  


-- 
Damien Le Moal
Western Digital Research