[PATCH 1/3] PCI: rockchip: split out rockchip_cfg_atu

[Shawn Lin]

在 2016/11/23 10:29, Brian Norris 写道:
> On Wed, Nov 23, 2016 at 10:15:16AM +0800, Shawn Lin wrote:
>> 在 2016/11/23 9:59, Brian Norris 写道:
>>> On Wed, Nov 23, 2016 at 09:19:11AM +0800, Shawn Lin wrote:
>>>> diff --git a/drivers/pci/host/pcie-rockchip.c b/drivers/pci/host/pcie-rockchip.c
>>>> index b55037a..19399fc 100644
>>>> --- a/drivers/pci/host/pcie-rockchip.c
>>>> +++ b/drivers/pci/host/pcie-rockchip.c
>
> ...
>
>>>> +	rockchip->msg_region = ioremap(rockchip->mem_bus_addr +
>>>> +				       ((reg_no - 1) << 20), SZ_1M);
>>>
>>> (And here.)
>>>
>>> ioremap() can fail; check for NULL.
>>>
>>
>> Sure.
>>
>>> Also, when you start reusing rockchip_cfg_atu() in patch 3, you'll be
>>> leaking virtual address space, as you'll keep remapping this every time.
>>
>> How about just check if rockchip->msg_region was already mapped?
>> Otherwise we don't remap it again when calling rockchip_cfg_atu.
>
> That'd work, even if it's a little awkward. That's basically one of my
> suggestions below.
>
>>> You should straighten that out. Either some kind of check for
>>> 'if (!rockchip->msg_region)', or just do the map/unmap where it's
>>> actually used (in patch 3).
>>
>> Should we really need to unmap it? As this driver won't be a module and
>> I think it's okay to keep the rockchip->msg_region always.
>
> No, I guess we don't really need to unmap it. I just meant, you could
> map/unmap every time you use it, or make sure you just map it once (and
> only once).
>

Got it.

> Also (if it helps), you could use devm_ioremap(), in case you ever do
> make it removable.

That sounds good. :)

>
> Brian
>
>
>


-- 
Best Regards
Shawn Lin