Re: [PATCH] dt: bindings: add bindings for ipq4019 wifi block

[Raja Mani <rmani@qti.qualcomm.com>]

On Wednesday 13 January 2016 07:53 AM, Rob Herring wrote:
> On Wed, Dec 30, 2015 at 11:41 PM, Raja Mani <rmani@qti.qualcomm.com> wrote:
>>
>> On Wednesday 30 December 2015 10:05 PM, Rob Herring wrote:
>>>
>>> On Wed, Dec 23, 2015 at 11:05:15AM +0530, Raja Mani wrote:
>>>>
>>>> Add device tree binding documentation details for wifi block present
>>>> in Qualcomm IPQ4019 SoC into qcom,ath10k.txt.
>>>>
>>>> Signed-off-by: Raja Mani <rmani@qti.qualcomm.com>
>>>> ---
>>>>    .../bindings/net/wireless/qcom,ath10k.txt          | 87
>>>> ++++++++++++++++++++--
>>>>    1 file changed, 82 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt
>>>> b/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt
>>>> index edefc26..ffd0742 100644
>>>> --- a/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt
>>>> +++ b/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt
>>>> @@ -1,17 +1,42 @@
>>>>    * Qualcomm Atheros ath10k wireless devices
>>>>
>>>> -For ath10k devices the calibration data can be provided through Device
>>>> -Tree. The node is a child node of the PCI controller.
>>>
>>>
>>> So it is now not a PCI device?
>>
>>
>> Right now, ath10k wireless driver has support for PCI devices. There is
>> a plan to extend ath10k driver to support wifi devices which are connected
>> over AHB as well (enumeration will happen via device tree node).
>>
>> Qualcomm IPQ4019 SoC has two inbuilt wifi block which are connected over AHB
>> bus (not connected over PCI bus) and also has one external PCI
>> slot where we can connect standalone wifi PCI devices.
>>
>> In future, ath10k would support both PCI and AHB. For PCI based
>> devices, only calibration data is supplied via device tree. For AHB
>> based devices (ie, ipq4019), all wifi properties are supplied
>> via device tree (including irq, reg addr, cal data,etc).
>
> Put this information in the binding. It is not clear and you seem to
> be removing PCI information.

Okay, I'll fix it in next version.

>
>>>> -
>>>>    Required properties:
>>>> --compatible : Should be "qcom,ath10k"
>>>> +- compatible: Should be one of the following:
>>>> +       * "qcom,ath10k"
>>>> +       * "qcom,ipq4019-wifi"
>>>
>>>
>>> One is a standalone PCI device and one is embedded block in an SOC?
>>
>>
>> Yes, it's possible to have this combination (one in SoC + one in
>> standalone PCI device in the same platform).
>>
>>> These should be more separated as all these new properties would only
>>> apply in the latter case.
>>
>>
>> Sorry, i didn't get this point. I mentioned it under optional
>> properties. Whichever properties applies to particular wifi, only
>> those parameters are needed. For example, for PCI based devices,
>> only calibration data is needed, for AHB based devices most the
>> properties are needed.
>>
>> Is it fine if add below text some thing like this ?
>>
>> "only "qcom,ath10k-calibration-data" is applicable for PCI based
>> devices, rest of the members are applicable only for AHB based
>> devices"
>
> Yes, but you need to explain in terms of compatible strings. Which
> compatible strings correspond to PCI devices.

Sure, i'll add this detail.

>
>>>>    Optional properties:
>>>> +- reg: Address and length of the register set for the device.
>>>> +- core-id: Core number associated to the device.
>>>
>>>
>>> This needs a better explanation.
>>
>>
>> Sure, Let me add below text in next version.
>>
>> "core-id is numeric number associated to the wifi block.
>>   For example, 0 means first block, 1 means second wifi block,etc."
>
> Are the blocks the same? If not, then use different compatible
> strings. Otherwise doesn't the reg property identify which block is
> which? Or some other property? Different freq bands? We generally
> don't allow indexes like this in the DT, so I need to understand how
> you use this.
>
> Rob
>

I understand the confusion and the limitation, i'll remove core-id in 
next version and manage this in driver itself.

Could you please consider v3 of this patch for further review? I hope,
i addressed all your review comments.

--
Raja