Re: [PATCH 1/3] dt-bindings: wireless: add ath11k pcie bindings

Johan Hovold [off-list ref] writes:

On Mon, Mar 20, 2023 at 08:41:21PM +0200, Kalle Valo wrote:

quoted

+ ath11k list

Johan Hovold [off-list ref] writes:

quoted

On Mon, Mar 20, 2023 at 02:22:12PM +0200, Kalle Valo wrote:

quoted

Johan Hovold [off-list ref] writes:

quoted

Add devicetree bindings for Qualcomm ath11k PCIe devices such as WCN6856
for which the calibration data variant may need to be described.

Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 .../bindings/net/wireless/pci17cb,1103.yaml   | 56 +++++++++++++++++++
 1 file changed, 56 insertions(+)
 create mode 100644
Documentation/devicetree/bindings/net/wireless/pci17cb,1103.yaml

I'm confused (as usual), how does this differ from
bindings/net/wireless/qcom,ath11k.yaml? Why we need two .yaml files?

Almost none of bindings/net/wireless/qcom,ath11k.yaml applies to WCN6856
when using PCIe (e.g. as most properties are then discoverable).

We could try to encode everything in one file, but that would likely
just result in a big mess of a schema with conditionals all over.

Ah, so the current qcom,ath11k.yaml would be only for ath11k AHB devices
and this new file is only for ath11k PCI devices?

Right, there would two separate schema files for the two device classes.

quoted

But why still the odd
name pci17cb,1103.yaml? It's not really descriptive and I'm for sure
will not remember that pci17cb,1103.yaml is for ath11k :)

Yeah, it's not the best name from that perspective, but it follows the
current convention of naming the schema files after the first compatible
added.

That said, we don't have many schemas for PCI devices so perhaps we can
establish a new convention for those. Perhaps by replacing the numerical
ids with what we'd use if these were platform devices (e.g.
'qcom,wcn6855.yaml').

As long as the DT maintainers are OK with it, I'd also be happy with
something like you suggest below:

	qcom,ath11k-ahb.yaml
	qcom,ath11k-pci.yaml

(or simply not renaming the current file 'qcom,ath11k.yaml') but I have
gotten push back on that in the past.

Ok, maybe it's then better not to try renaming qcom,ath11k.yaml and keep
it as is.

quoted

Also it doesn't look good that we have qcom,ath11k-calibration-variant
documented twice now. I'm no DT expert but isn't there any other way? Is
it possible to include other files? For example, if we would have three
files:

qcom,ath11k.yaml
qcom,ath11k-ahb.yaml
qcom,ath11k-pci.yaml

Then have the common properties like ath11k-calibration-variant in the
first file and ahb/pci files would include that.

That should be possible, but it's not necessarily better as you'd then
have to look up two files to see the bindings for either device class
(and as far as I can tell there would not be much sharing beyond this
single property).

Note that the property could just have well have been named
'qcom,calibration-variant' and then it would be shared also with the
ath10k set of devices which currently holds another definition of what
is essentially the same property ('qcom,ath10k-calibration-variant').

Oh man, having it as 'qcom,calibration-variant' would have been so much
better. Oh well, too late now :(

Thanks for explaining all this.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches