Re: [PATCH v2 6/8] ath10k: implement ath10k_pci_soc_read/write32()

[Kalle Valo <kvalo@qca.qualcomm.com>]

Michal Kazior <michal.kazior@tieto.com> writes:

> On 16 October 2013 06:46, Kalle Valo <kvalo@qca.qualcomm.com> wrote:
>> To make it easier to access SOC registers. No functional
>> changes.
>>
>> Signed-off-by: Kalle Valo <kvalo@qca.qualcomm.com>
>> ---
>>  drivers/net/wireless/ath/ath10k/pci.c |    3 +--
>>  drivers/net/wireless/ath/ath10k/pci.h |   10 ++++++++++
>>  2 files changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/pci.c b/drivers/net/wireless/ath/ath10k/pci.c
>> index d09f8a2..5c78383 100644
>> --- a/drivers/net/wireless/ath/ath10k/pci.c
>> +++ b/drivers/net/wireless/ath/ath10k/pci.c
>> @@ -2454,8 +2454,7 @@ static int ath10k_pci_probe(struct pci_dev *pdev,
>>                 return ret;
>>         }
>>
>> -       chip_id = ath10k_pci_read32(ar,
>> -                                   RTC_SOC_BASE_ADDRESS + SOC_CHIP_ID_ADDRESS);
>> +       chip_id = ath10k_pci_soc_read32(ar, SOC_CHIP_ID_ADDRESS);
>>
>>         ath10k_do_pci_sleep(ar);
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/pci.h b/drivers/net/wireless/ath/ath10k/pci.h
>> index 52fb7b9..a304c33 100644
>> --- a/drivers/net/wireless/ath/ath10k/pci.h
>> +++ b/drivers/net/wireless/ath/ath10k/pci.h
>> @@ -318,6 +318,16 @@ static inline u32 ath10k_pci_read32(struct ath10k *ar, u32 offset)
>>         return ioread32(ar_pci->mem + offset);
>>  }
>>
>> +static inline u32 ath10k_pci_soc_read32(struct ath10k *ar, u32 addr)
>> +{
>> +       return ath10k_pci_read32(ar, RTC_SOC_BASE_ADDRESS + addr);
>> +}
>> +
>> +static inline void ath10k_pci_soc_write32(struct ath10k *ar, u32 addr, u32 val)
>> +{
>> +       ath10k_pci_write32(ar, RTC_SOC_BASE_ADDRESS + addr, val);
>> +}
>> +
>
> I'm not entirely sure about this. There are a couple of soc address
> groups (RTC_SOC, RTC_WMAC, SOC_PCIE, SOC_CORE, ..).

And then we can have different functions for each group.

> I'd rather use just raw ioread/iowrite than have all those wrappers.

Well, I again would prefer to have clean interfaces instead of doing
address arithmetic in every call :)

> I think the reason for having ath10k_pci_{read,write}32 was the HW v1
> workaround.

No, it was also to make the code simple.

> Do we really need to keep it?

In my opinion yes. What is the negative side of keeping them? Why don't
you like these wrappers?

-- 
Kalle Valo

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k