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

[Raja Mani]

On Wednesday 13 January 2016 07:53 AM, Rob Herring wrote:
> On Wed, Dec 30, 2015 at 11:41 PM, Raja Mani <rmani@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxx>
> > > > ---
> > > >    .../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