Am 01.03.2018 um 21:15 schrieb Andy Shevchenko:
> On Thu, 2018-03-01 at 20:54 +0100, Heiner Kallweit wrote:
>> Am 01.03.2018 um 12:27 schrieb Andy Shevchenko:
>
>>> - rc = pci_request_regions(pdev, MODULENAME);
>>> + rc = pcim_iomap_regions(pdev, BIT(region), MODULENAME);
>>> if (rc < 0) {
>>> - netif_err(tp, probe, dev, "could not request
>>> regions\n");
>>> + netif_err(tp, probe, dev, "cannot remap MMIO,
>>> aborting\n");
>>> return rc;
>>> }
>>>
>>>
>
>>> + tp->mmio_addr = pcim_iomap_table(pdev)[region];
>>>
>>
>> pcim_iomap_table() can return NULL in case of an error.
>
> No.
>
>> Shouldn't we catch this?
>
> No.
>
> Yeah, I'm a bit tired to explain everyone that pcim_iomap_table() will
> never fail if previous pcim_iomap_regions() not failed.
>
Ahh, missed this because I didn't dig that deep into these functions.
Would be nice if pcim_iomap_regions() returned iomap directly
(or an ERRPTR). Then the additional call to pcim_iomap_table()
wouldn't be needed.
>> Typical benefit of switching to device-managed functions is that we
>> don't
>> have to clean up in the probe() error path and in remove(). With the
>> change here we don't have any such benefit and we just exchange two
>> calls
>> against two other calls w/o functional change (AFAICS).
>> Which benefit do you see justifying this patch?
>
> -67% statistics is a good sign, no?
>
>> However I don't know the PCI API's good enough to be able to judge
>> whether
>> one set of calls is preferable.
>
> More than above, it's about consistency. While you switch to devm_, for
> PCI driver it's naturally to go for pcim_.
>
> Esp. taking into account that pcim_enable_device() _is_ already there.
>
