Re: [RFC PATCH 3/3] libata-sff: Simplify request of PCI resources

[Sergey Shtylyov <s.shtylyov@omp.ru>]

On 12/4/24 8:10 PM, Philipp Stanner wrote:

> pcim_iomap_regions() has been deprecated by the PCI subsystem.
> 
> Unfortunately, libata-sff uses quite complicated bit mask magic to
> obtain its PCI resources.
> 
> Restructure and simplify the PCI resource request code.
> 
> Replace pcim_iomap_regions() with pcim_iomap_region().
> 
> Signed-off-by: Philipp Stanner <pstanner@redhat.com>
[...]

> diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
> index 67f277e1c3bf..1d2273c0f447 100644
> --- a/drivers/ata/libata-sff.c
> +++ b/drivers/ata/libata-sff.c
> @@ -2127,6 +2127,73 @@ static bool ata_resources_present(struct pci_dev *pdev, int port)
>  	return true;
>  }
>  
> +static void ata_pci_sff_set_ap_data(struct ata_port *ap, struct ata_host *host,

   Maybe ata_pci_sff_set_port_data()?

> +		struct pci_dev *pdev, unsigned short base)

   Using just 2 tabs here seems to go against the coding style used in libata-sff.c
but no biggie... :-)

> +{
> +	void __iomem *ctl_addr;
> +
> +	ctl_addr = host->iomap[base + 1];
> +	ctl_addr = (void __iomem *)((unsigned long)ctl_addr | ATA_PCI_CTL_OFS);
> +
> +	ap->ioaddr.cmd_addr = host->iomap[base];
> +	ap->ioaddr.altstatus_addr = ctl_addr;
> +	ap->ioaddr.ctl_addr = ctl_addr;
> +
> +	ata_sff_std_ports(&ap->ioaddr);
> +
> +	ata_port_desc(ap, "cmd 0x%llx ctl 0x%llx",
> +		(unsigned long long)pci_resource_start(pdev, base),
> +		(unsigned long long)pci_resource_start(pdev, base + 1));
> +}
> +
> +/*

   Please use /** here -- the comment seems to otherwise match the kernel-doc
format...

> + * ata_pci_sff_obtain_bars - obtain the PCI BARs associated with an ATA port
> + * @pdev: the PCI device
> + * @host: the ATA host
> + * @ap: the ATA port
> + * @port: @ap's port index in @host
> + *
> + * Returns: Number of successfully ioremaped BARs, a negative code on failure
> + */
> +static int ata_pci_sff_obtain_bars(struct pci_dev *pdev, struct ata_host *host,
> +		struct ata_port *ap, unsigned short port)
> +{
> +	int ret = 0, bars_mapped = 0;
> +	unsigned short i, base;
> +	void __iomem *io_tmp;

   Maybe call it iomem instead?

> +	const char *name = dev_driver_string(&pdev->dev);
> +
> +	/*
> +	 * Port can be 0 or 1.
> +	 * Port 0 corresponds to PCI BARs 0 and 1, port 1 to BARs 2 and 3.
> +	 */
> +	base = port * 2;
> +
> +       /*

   Don't indent with spaces please.

> +	* Discard disabled ports. Some controllers show their unused channels
> +	* this way. Disabled ports are made dummy.
> +	*/

   These 3 lines lack a space before *.

[...]
> +	for (i = 0; i < 2; i++) {
> +		io_tmp = pcim_iomap_region(pdev, base + i, name);
> +		ret = PTR_ERR_OR_ZERO(io_tmp);
> +		if (ret != 0)

   != 0 unnecessary.

[...]
> @@ -2148,59 +2215,31 @@ static bool ata_resources_present(struct pci_dev *pdev, int port)
[...]> -	if (!mask) {
> -		dev_err(gdev, "no available native port\n");
> +	if (operational_ports == 0)

   Perhaps:

	if (!operational_ports)

[...]

MBR, Sergey